Fix command line argument handling for slice configuration parameters #299
Conversation
… (comma separated list).
|
thx let me take a look |
|
ci not pass |
| @@ -103,9 +103,9 @@ func TestConfigRuntimeArgs(t *testing.T) { | |||
| }, | |||
| { | |||
| name: "check exclude_regex", | |||
There was a problem hiding this comment.
can we add the test case which is empty slice?
There was a problem hiding this comment.
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.
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 util.go to explicitly set an empty slice), but that will override all the default values because arguments that are not explicitly set seem to end up as empty strings instead of nil. Do you think it would be easy to adjust that non-set arguments would be nil instead of empty strings? I don't know the code base well enough yet and I have not so much experience with the flag package.
There was a problem hiding this comment.
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 nil as default value).
See also here https:/cosmtrek/air/blob/master/runner/flag.go#L12.
There was a problem hiding this comment.
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.
Thanks for your support. I cherry-picked all 3 commits.
|
@the use comma to split string as slice is a good idea! plz fix the lint, and add the test case which include empty string, and seems should add more description in README. after that, I will merge this PR. thanks |
I'm puzzled about the errors: |
Codecov Report
@@ Coverage Diff @@
## master #299 +/- ##
==========================================
+ Coverage 66.29% 66.91% +0.61%
==========================================
Files 6 7 +1
Lines 905 943 +38
==========================================
+ Hits 600 631 +31
- Misses 241 246 +5
- Partials 64 66 +2
Continue to review full report at Codecov.
|
I have rerunned the failed test, It's passed. the failed pipeline seems not relate with this version. ignore it. |
I added a small note in the README. |
| @@ -30,6 +30,10 @@ Air 是为 Go 应用开发设计的另外一个热重载的命令行工具。只 | |||
|
|
|||
| `air --build.cmd "go build -o bin/api cmd/run.go" --build.bin "./bin/api"` | |||
|
|
|||
| 对于以列表形式输入的参数，使用逗号来分隔项目: | |||
It seems slice configuration parameters such as
ExcludeDircannot be set correctly with command line arguments (nor override default config). The test for--build.exclude_regexonly passes because the test value corresponds with the default value (set indefaultConfig), it's not actually set by the parameter in the test.I propose to treat the command line arguments for such config parameters as comma separated strings. Setting a value would work as e.g.
--build.exclude_dir "templates,build".