Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

guest/generate: change --append option to be a boolean based on presence #66

Merged

Conversation

erichte-ibm
Copy link
Collaborator

Fixes #65.

Currently, the -a --append option accepts a string to be parsed into an integer, either 0 or 1. This integer is then used to set a boolean append flag.

Avoid parsing strings at all by having the boolean append flag be set to 1 if -a/--append exists, otherwise default to 0.

NOTE: This does introduce a change in the command line API.


This one is a slightly more invasive change, but I think the removal of needless string parsing is better for overall safety. Please let me know if you disagree with this approach.

Fixes open-power#65.

Currently, the -a --append option accepts a string to be parsed into
an integer, either 0 or 1. This integer is then used to set a boolean
append flag.

Avoid parsing strings at all by having the boolean append flag be set
to true if -a/--append exists, otherwise default to false.

NOTE: This does introduce a change in the command line API.

Signed-off-by: Eric Richter <erichte@linux.ibm.com>
@nick-child-ibm
Copy link
Collaborator

I think the API breakage is worth the fix. I agree with your argument.

Though we really need to add a test for this flag being properly applied in our test suite.
I WAS able to manually run this patch and use validate to observe that it does properly detect the presence of the append flag.
So the patch is working and we can delay the addition of a test case until we get around to increasing code coverage.

As always, thank you!

@nick-child-ibm nick-child-ibm merged commit adec995 into open-power:guest-devel Oct 5, 2023
8 checks passed
@erichte-ibm erichte-ibm mentioned this pull request Oct 12, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants