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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Apr 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f37d1d
Fix(1199). Allow -ve values for int, float & duration
dearchap Apr 3, 2021
d198aed
Add test when flag is not set
dearchap Apr 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 7 additions & 13 deletions altsrc/flag.go
View file
Open in desktop
Expand Up @@ -197,10 +197,8 @@ func (f *IntFlag) ApplyInputSourceValue(cCtx *cli.Context, isc InputSourceContex
if err != nil {
return err
}
if value > 0 {
Copy link
Member

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

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

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

Copy link

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?

for _, name := range f.Names() {
_ = f.set.Set(name, strconv.FormatInt(int64(value), 10))
}
for _, name := range f.Names() {
_ = f.set.Set(name, strconv.FormatInt(int64(value), 10))
}
}
}
Expand All @@ -215,10 +213,8 @@ func (f *DurationFlag) ApplyInputSourceValue(cCtx *cli.Context, isc InputSourceC
if err != nil {
return err
}
if value > 0 {
for _, name := range f.Names() {
_ = f.set.Set(name, value.String())
}
for _, name := range f.Names() {
_ = f.set.Set(name, value.String())
}
}
}
Expand All @@ -233,11 +229,9 @@ func (f *Float64Flag) ApplyInputSourceValue(cCtx *cli.Context, isc InputSourceCo
if err != nil {
return err
}
if value > 0 {
floatStr := float64ToString(value)
for _, name := range f.Names() {
_ = f.set.Set(name, floatStr)
}
floatStr := float64ToString(value)
for _, name := range f.Names() {
_ = f.set.Set(name, floatStr)
}
}
}
Expand Down
36 changes: 36 additions & 0 deletions altsrc/flag_test.go
View file
Open in desktop
Expand Up @@ -234,6 +234,15 @@ func TestIntApplyInputSourceMethodSet(t *testing.T) {
expect(t, 15, c.Int("test"))
}

func TestIntApplyInputSourceMethodSetNegativeValue(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewIntFlag(&cli.IntFlag{Name: "test"}),
FlagName: "test",
MapValue: -1,
})
expect(t, -1, c.Int("test"))
}

func TestIntApplyInputSourceMethodContextSet(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewIntFlag(&cli.IntFlag{Name: "test"}),
Expand Down Expand Up @@ -264,6 +273,15 @@ func TestDurationApplyInputSourceMethodSet(t *testing.T) {
expect(t, 30*time.Second, c.Duration("test"))
}

func TestDurationApplyInputSourceMethodSetNegativeValue(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewDurationFlag(&cli.DurationFlag{Name: "test"}),
FlagName: "test",
MapValue: -30 * time.Second,
})
expect(t, -30*time.Second, c.Duration("test"))
}

func TestDurationApplyInputSourceMethodContextSet(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewDurationFlag(&cli.DurationFlag{Name: "test"}),
Expand Down Expand Up @@ -294,6 +312,24 @@ func TestFloat64ApplyInputSourceMethodSet(t *testing.T) {
expect(t, 1.3, c.Float64("test"))
}

func TestFloat64ApplyInputSourceMethodSetNegativeValue(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewFloat64Flag(&cli.Float64Flag{Name: "test"}),
FlagName: "test",
MapValue: -1.3,
})
expect(t, -1.3, c.Float64("test"))
}

func TestFloat64ApplyInputSourceMethodSetNegativeValueNotSet(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewFloat64Flag(&cli.Float64Flag{Name: "test1"}),
FlagName: "test1",
// dont set map value
})
expect(t, 0.0, c.Float64("test1"))
}

func TestFloat64ApplyInputSourceMethodContextSet(t *testing.T) {
c := runTest(t, testApplyInputSource{
Flag: NewFloat64Flag(&cli.Float64Flag{Name: "test"}),
Expand Down