Skip to content
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

Assert and convert config value into predefined struct #1440

Merged
merged 1 commit into from
Jul 7, 2015

Conversation

rht
Copy link
Contributor

@rht rht commented Jul 2, 2015

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.

@jbenet jbenet added the backlog label Jul 2, 2015
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it will just be less confusing overall to require using --json

@rht rht force-pushed the fix-cli branch 2 times, most recently from 60b6956 to bf6e7c5 Compare July 4, 2015 14:19
@rht
Copy link
Contributor Author

rht commented Jul 4, 2015

RFM. There was only mock test err.

castedValue = value
}
if !ok {
return fmt.Errorf("Wrong config type, expected %T", oldValue)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is unchanged.

@jbenet
Copy link
Member

jbenet commented Jul 5, 2015

#1440 (comment)

@jbenet
Copy link
Member

jbenet commented Jul 7, 2015

ok thanks @rht

jbenet added a commit that referenced this pull request Jul 7, 2015
Assert and convert config value into predefined struct
@jbenet jbenet merged commit 6c092eb into ipfs:master Jul 7, 2015
@jbenet jbenet removed the backlog label Jul 7, 2015
@rht rht deleted the fix-cli branch July 7, 2015 03:04
Stebalien added a commit that referenced this pull request Jun 16, 2020
This bug was pointed out in
#1440 (comment) but was ignored
for some reason.
Stebalien added a commit that referenced this pull request Jun 16, 2020
This bug was pointed out in
#1440 (comment) but was ignored
for some reason.
Stebalien added a commit that referenced this pull request Jun 16, 2020
This bug was pointed out in
#1440 (comment) but was ignored
for some reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants