Assert and convert config value into predefined struct #1440
Conversation
| @@ -65,7 +65,7 @@ ipfs daemon --mount | |||
| If you wish to allow other users to use the mount points, use the following: | |||
|
|
|||
| ```sh | |||
| ipfs config Mounts.FuseAllowOther --bool true | |||
There was a problem hiding this comment.
Ok, so here is a clear case of what i'm talking about:
ipfs config Mounts.FuseAllowOther true # bool
ipfs config Mounts.FuseAllowOther "true" # bool
ipfs config Mounts.FuseAllowOther '"true"' # string
if i wanted the value "true", i would expect the 2nd line to work.
There was a problem hiding this comment.
i think it will just be less confusing overall to require using --json
rht
force-pushed
the
fix-cli
branch
2 times, most recently
from
60b6956
to
bf6e7c5
Compare
|
RFM. There was only mock test err. |
| castedValue = value | ||
| } | ||
| if !ok { | ||
| return fmt.Errorf("Wrong config type, expected %T", oldValue) |
There was a problem hiding this comment.
This entire switch-case can be moved to common if we expect the values in the "json object" to not change in type. Wish could be cleaner.
License: MIT Signed-off-by: rht <rhtbot@gmail.com>
| if !ok { | ||
| return fmt.Errorf("Wrong config type, expected %T", oldValue) | ||
| } | ||
| } |
There was a problem hiding this comment.
i think this is actually bad-- if i edit a config and accidentally set the wrong type value, we cannot fix it from the CLI.
this is getting complicated. let's just assume string and require --json for json parsing.
There was a problem hiding this comment.
The "if i ..." case here is very specific to user-specified value.
But sure I can remove the granular type checks, although there is already a cruder version of type-check after those lines (which does similar things but doesn't pinpoint exactly where the err is).
There was a problem hiding this comment.
ok. to confirm, want:
ipfs config <key> [1,2,3] # set as string
ipfs config <key> "[1,2,3]" # set as string
ipfs config <key> --json [1,2,3] # set as array of numbers
|
ok thanks @rht |
Assert and convert config value into predefined struct
This bug was pointed out in #1440 (comment) but was ignored for some reason.
This bug was pointed out in #1440 (comment) but was ignored for some reason.
This bug was pointed out in #1440 (comment) but was ignored for some reason.
Basically with this PR, 'true' is parsed as boolean, making explicit '--bool'/'--number'/'--string' optional if there is already config struct.
Lower dose version of #1355 that doesn't break backward compatibility.