-
-
Notifications
You must be signed in to change notification settings - Fork 806
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
Fix command line argument handling for slice configuration parameters #299
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,9 +103,18 @@ func TestConfigRuntimeArgs(t *testing.T) { | |
}, | ||
{ | ||
name: "check exclude_regex", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add the test case which is empty slice? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm what would you expect in that case, the default values ore empty slice? It seems currently it keeps the default values (I didn't think of this case before), but I'd rather expect to set it to empty. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer set it to empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately I think it isn't that straightforward. Before a value is set there's a check for nil and empty string https:/cosmtrek/air/blob/master/runner/config.go#L324. If I only keep the nil check it would work (with an additional change in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I see correctly it's only possible to define string variable flags with a string as default value (not a pointer, thus impossible to set See also here https:/cosmtrek/air/blob/master/runner/flag.go#L12. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can set default string as unset value https:/cosmtrek/air/pull/300/files#diff-19e19f16fa5018b7ad46fc960cdbe0138b0029dac020b42c798a9ff9b8203493R7 which is user use to set. then filter the unset value in there https:/cosmtrek/air/pull/300/files#diff-0a9b65ef9e6a4d3b9e2aec07d2dbaed8bb3866d74d3a2c78d78ddf56ed9aa071L324 u can cherry-pick my commit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for your support. I cherry-picked all 3 commits. |
||
args: []string{"--build.exclude_regex", `["_test.go"]`}, | ||
args: []string{"--build.exclude_regex", "_test.go,.html"}, | ||
check: func(t *testing.T, conf *Config) { | ||
assert.Equal(t, []string{"_test.go"}, conf.Build.ExcludeRegex) | ||
assert.Equal(t, []string{"_test.go", ".html"}, conf.Build.ExcludeRegex) | ||
}, | ||
}, | ||
{ | ||
name: "check exclude_regex with empty string", | ||
args: []string{"--build.exclude_regex", ""}, | ||
check: func(t *testing.T, conf *Config) { | ||
assert.Equal(t, []string{}, conf.Build.ExcludeRegex) | ||
t.Logf("%+v", conf.Build.ExcludeDir) | ||
assert.NotEqual(t, []string{}, conf.Build.ExcludeDir) | ||
}, | ||
}, | ||
} | ||
|
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.
great!