fix: Avoid ParseBool error #2011
Conversation
The error occurs if I set "yes" to `FORCE_PREPEND` env as described in the documentation. Error message: ``` strconv.ParseBool: parsing \"yes\": invalid syntax ``` Referenced Documents: - https://github.com/asdf-vm/asdf/blob/7352bf4890143184ba419392ca5e6204167c8306/docs/manage/configuration.md#asdf_force_prepend
machupicchubeta
force-pushed
the
force_prepend
branch
from
654ba96 to
82c9dc0
Compare
internal/config/config_test.go
Outdated
|
|
||
| assert.Nil(t, err, "Returned error when loading env for config") | ||
|
|
||
| assert.Zero(t, config.Home, "Shouldn't set Home property when loading config") |
There was a problem hiding this comment.
This test should be updated to check whether ForcePrepend was actually set to the correct/expected value. Also consider adding a test for the default case (where ASDF_FORCE_PREPEND is not set).
There was a problem hiding this comment.
Thank you for your comment.
I have changed the test code to check that ForcePrepend is set the correct/expected value.
And I have check the initial value when ASDF_FORCE_PREPEND is not set.
There was a problem hiding this comment.
@andrecloutier
By updating the test cases, I was able to notice where there were discrepancies with the specifications, and I updated the code to include those as well.
Would you please code review again?
There was a problem hiding this comment.
Looking good! Added a few minor comments.
By the way... From searching the code, it looks like nothing is reading from ForcePrepend. So, while this PR will fix the parsing of the value, this won't actually result in any visible change in behavior in the application.
There was a problem hiding this comment.
@andrecloutier
Thank you so much.
There is nothing is reading ForcePrepend .... Oh, indeed. I noticed that when you mentioned it.
I will consider that as another issue.
The reason I decided to write this code in the first place was because I saw the error on my local terminal.
For some reason I can't reproduce it now (.... hmm), but I hope to be able to prevent similar case.
…ironment variable Those test cases aims to check whether `ForcePrepend` was actually set to the correct/expected value. Referenced Documents: - https://github.com/asdf-vm/asdf/blob/7352bf4890143184ba419392ca5e6204167c8306/docs/manage/configuration.md#asdf_force_prepend
machupicchubeta
force-pushed
the
force_prepend
branch
3 times, most recently
from
559004e to
b017252
Compare
If the `ASDF_FORCE_PREPEND` environment variable is not set on macOS, the behavior is the same as when "yes" is set. Referenced Documents: - https://github.com/asdf-vm/asdf/blob/7352bf4890143184ba419392ca5e6204167c8306/docs/manage/configuration.md#asdf_force_prepend
`ASDF_FORCE_PREPEND` is a specification that changes behavior depending on whether the string is "yes" or not. Referenced Documents: - https://github.com/asdf-vm/asdf/blob/7352bf4890143184ba419392ca5e6204167c8306/docs/manage/configuration.md#asdf_force_prepend
machupicchubeta
force-pushed
the
force_prepend
branch
from
b017252 to
934f15d
Compare
| forcePrepend = true | ||
| default: | ||
| forcePrepend = false | ||
| } |
There was a problem hiding this comment.
I think the use of switch here adds unnecessary complexity and branches to consider. I suggest using an if instead:
forcePrepend := forcePrependDefault
forcePrependEnv := os.Getenv("ASDF_FORCE_PREPEND")
if forcePrependEnv == "yes" || (forcePrependEnv == "" && runtime.GOOS == "darwin") {
forcePrepend = true
}
There was a problem hiding this comment.
Yes. that's exactly it!
Thank you!
internal/config/config_test.go
Outdated
| func TestLoadConfigEnv_WithForcePrependEnv(t *testing.T) { | ||
| t.Run("When ASDF_FORCE_PREPEND env given yes", func(t *testing.T) { | ||
| os.Setenv("ASDF_FORCE_PREPEND", "yes") | ||
| defer os.Unsetenv("ASDF_FORCE_PREPEND") |
There was a problem hiding this comment.
Using t.SetEnv in a substitute to having to do any special defer handling. (In fact, there's a bug here where ASDF_FORCE_PREPEND should be set back to the previous value if it were set to one before).
There was a problem hiding this comment.
I see.
Thank you very much!
I fix the code.
internal/config/config_test.go
Outdated
|
|
||
| t.Run("When ASDF_FORCE_PREPEND env given any string other than yes", func(t *testing.T) { | ||
| os.Setenv("ASDF_FORCE_PREPEND", "no") | ||
| defer os.Unsetenv("ASDF_FORCE_PREPEND") |
There was a problem hiding this comment.
It is the same as #2011 (comment).
Summary
The error occurs if I set "yes" to
FORCE_PREPENDenv as described in the documentation.Error message
Other Information
Referenced Documents