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.
can we add the test case which is empty slice?
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.
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 comment
The 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 comment
The 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
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 ofnil
. Do you think it would be easy to adjust that non-set arguments would benil
instead of empty strings? I don't know the code base well enough yet and I have not so much experience with theflag
package.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.
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.
Choose a reason for hiding this comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your support. I cherry-picked all 3 commits.