fixed dm and Makefile #105
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oracle-contributor-agreement
bot
added
the
OCA Verified
brenns10
requested changes
Oct 14, 2024
There was a problem hiding this comment.
A few things in addition to the two comments:
- Can you file an (Oracle) bug for this change? And can you say whether the dm fix should be backported to v1? If so, let's file both bugs for that.
- I see the merge commit in there, I try to avoid merge commits wherever possible. Could you get rid of that?
- Would you mind splitting out the
dm.pychange and theMakefilecommits into their own separate commits? I don't mind them being in the same pull request, but they are logically different changes and shouldn't live in the same commit.
Makefile
Outdated
| rsync -avz --exclude "__pycache__" --exclude ".git" --exclude ".mypy_cache" ./drgn_tools $(TARGET):drgn_tools/ | ||
| rsync -avz --exclude "__pycache__" --exclude ".git" --exclude ".mypy_cache" ./drgn_tools $(TARGET) |
There was a problem hiding this comment.
To ensure that this doesn't break any existing users who may already have TARGET set and expect it to go to TARGET:drgn_tools/, would you please go ahead and add the following code into the Makefile? Directly beneath the -include config.mk directive would be best.
# The TARGET variable could be either a hostname or a hostname:path. Previously
# the makefile expected just a hostname, and it always placed the resulting
# directory at $TARGET:drgn_tools/. But it's nice to have flexibility to change
# the target directory, so we only add that default path if there's not already
# a path specified.
ifneq (,$(TARGET))
ifeq (,$(findstring :,$(TARGET)))
override TARGET := $(TARGET):drgn_tools/
else
endif
endif
drgn_tools/dm.py
Outdated
Comment on lines
36
to
44
| yield hc.md, hc.name.string_().decode(), hc.uuid.string_().decode() | ||
| uuid = "" | ||
| if hc.uuid: | ||
| uuid = hc.uuid.string_().decode() | ||
|
|
||
| yield hc.md, hc.name.string_().decode(), uuid |
There was a problem hiding this comment.
The fixes look good, but the commit message doesn't contain any context for why they are necessary. Was there a change that made the uuid field nullable, or has it always been and we just didn't handle it?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.