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

Allow -ve values for int, float & duration #1262

Merged
merged 2 commits into from Apr 24, 2022

Conversation

dearchap
Copy link
Contributor

@dearchap dearchap commented Apr 3, 2021

What type of PR is this?

  • bug

What this PR does / why we need it:

This PR allows negative values for int64, float64 and duration flags from alternate input sources

Which issue(s) this PR fixes:

Fixes #1199

Special notes for your reviewer:

Testing

Added 3 tests

  • TestIntApplyInputSourceMethodSetNegativeValue
  • TestDurationApplyInputSourceMethodSetNegativeValue
  • TestFloat64ApplyInputSourceMethodSetNegativeValue

Release Notes


@dearchap dearchap requested a review from a team as a code owner Apr 3, 2021
@dearchap dearchap requested review from rliebz and coilysiren and removed request for a team Apr 3, 2021
@@ -197,10 +197,8 @@ func (f *IntFlag) ApplyInputSourceValue(context *cli.Context, isc InputSourceCon
if err != nil {
return err
}
if value > 0 {
Copy link
Member

@rliebz rliebz Apr 8, 2021

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 want isc.Int to return value, 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 report false for IsSet, which is closer to the current behavior, and, if I had to guess, more likely than the case where 0 is explicitly set.

Copy link
Contributor Author

@dearchap dearchap Apr 8, 2021

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.

Copy link
Contributor Author

@dearchap dearchap Apr 24, 2022

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

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?

@coilysiren coilysiren requested review from a team and removed request for coilysiren Apr 23, 2021
@stale
Copy link

stale bot commented Jul 28, 2021

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

@stale stale bot added the status/stale stale due to the age of it's last update label Jul 28, 2021
@stale
Copy link

stale bot commented Aug 28, 2021

Closing this as it has become stale.

@stale stale bot closed this Aug 28, 2021
@dearchap
Copy link
Contributor Author

dearchap commented Aug 28, 2021

@rliebz rliebz reopened this Aug 30, 2021
@stale
Copy link

stale bot commented Aug 30, 2021

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

@stale stale bot removed the status/stale stale due to the age of it's last update label Aug 30, 2021
@albttx
Copy link

albttx commented Apr 8, 2022

Can this be updated and merge ?!

cc: @dearchap @urfave

@dearchap
Copy link
Contributor Author

dearchap commented Apr 10, 2022

@albttx i'll take a look at it this week

@meatballhat meatballhat changed the title Fix(1199). Allow -ve values for int, float & duration Allow -ve values for int, float & duration Apr 21, 2022
@meatballhat meatballhat added kind/bug describes or fixes a bug area/v2 relates to / is being considered for v2 labels Apr 21, 2022
@meatballhat meatballhat added this to the Release 2.5.0 milestone Apr 21, 2022
@meatballhat meatballhat changed the base branch from master to main Apr 21, 2022
Copy link
Member

@meatballhat meatballhat left a comment

Rad! Thanks! 🙇🏼 🎉

@meatballhat meatballhat merged commit 59ec2a1 into urfave:main Apr 24, 2022
12 checks passed
@albttx
Copy link

albttx commented Apr 25, 2022

Amazing, thanks 👍

@dearchap dearchap deleted the issue_1199 branch Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v2 relates to / is being considered for v2 kind/bug describes or fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integer negative numbers are returned zero when parsed from yaml file
5 participants