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
Allow -ve values for int, float & duration #1262
Merged
+43
−13
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to tell from the commit that introduced this check, because it happens to also be the commit that introduced YAML parsing, but I assume the intent here was to avoid calling
Set
on the flag set if the value wasn't explicitly specified. In an ideal world, I think we'd wantisc.Int
to returnvalue, ok, err
, so we would know whether the value was explicitly set as the zero value, versus just defaulting to zero.While it's also an imperfect solution, what are your thoughts on keeping a check for
value != 0
instead of> 0
? We wouldn't know whether or not the value was actually set if it's zero, but the key difference in behavior is that we would reportfalse
forIsSet
, which is closer to the current behavior, and, if I had to guess, more likely than the case where0
is explicitly set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually thinking about it a bit more we want to check that the value from yaml is not the same as the default value for that flag and only then set it. Then this would take care of both the issues. i.e we would report IsSet only if the value from yaml is different from flag default and also allow for range of values. This way the value from yaml could be zero as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rliebz Looking at it further the isc.XXX(f.Name) will return an error if the flag isnt found in the input source. So there is no chance of the Set being called in that case. Added a small unit test to verify that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI #1373
I don't know if this is my code causing the problem, or the change in this PR, and I'm not knowledgeable enough to figure it out. Maybe you guys can find out?