Assume config value to be json-formatted in cli #1355

rht · 2015-06-11T14:35:08Z

This deprecates explicit '--bool' and '--json' flag.
The reason is, if explicit type has to be specified, then '--integer',
'--float' also need to be implemented. But json parser already knows how
to handle these types, so why not just reuse it.
ref: #740

GitCop · 2015-06-11T14:35:10Z

There were the following issues with your Pull Request

Commit: 6c33579
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>

Guidelines are available at: https://github.com/ipfs/community/blob/master/docs/commit-message.md Your feedback on GitCop is welcome on the following issue: ipfs/infra#23

This message was auto-generated by https://gitcop.com

License: MIT Signed-off-by: rht <rhtbot@gmail.com>

This deprecates explicit '--bool' and '--json' flag. The reason is, if explicit type has to be specified, then '--integer', '--float' also need to be implemented. But json parser already knows how to handle these types, so why not just use it. License: MIT Signed-off-by: rht <rhtbot@gmail.com>

whyrusleeping · 2015-06-11T19:55:08Z

we should re-run the tests here. everything appears to have failed

jbenet · 2015-06-11T20:34:05Z

core/commands/config.go

+			if err := json.Unmarshal([]byte(value), &jsonVal); err != nil {
+				err = fmt.Errorf("failed to unmarshal json. %s", err)
+				res.SetError(err, cmds.ErrNormal)
+				return


so always default to json? that makes sense, but does that mean that we now have to do:

# used to work > ipfs config Addresses.API /ip4/127.0.0.1/tcp/5001 # must now be > ipfs config Addresses.API "/ip4/127.0.0.1/tcp/5001"

?

On Thu, Jun 11, 2015 at 01:34:07PM -0700, Juan Batiz-Benet wrote:

must now be

ipfs config Addresses.API "/ip4/127.0.0.1/tcp/5001"

Implementations differ in how strictly they enforce this, but I've
seen a number of folks suggest that strings are not valid JSON root
objects (e.g. 1). That's also how it's written up in RFC 4627 2.

ipfs config Addresses.API /ip4/127.0.0.1/tcp/5001 still works in this case (golang/pkg/encoding/json).

So I will have to check which implementation (RFC 4627 vs ECMA-262 5 and newer) is among the majority today.

edit: from here on 'ECMA-262 5' is renamed RFC 7159 https://tools.ietf.org/html/rfc7159#section-2

ipfs config Addresses.API /ip4/127.0.0.1/tcp/5001 still works

ok great.

accept stand-alone strings, numbers (RFC 7159):

Go 1.3+

Major browsers (IE 8+, everything else not IE)[1]

Python 2.7+[1]

else (RFC 4627):

Ruby 2.1.1 stdlib "json" [2]

Pending:
http://json.org/ (150-ish of those)

[1] http://stackoverflow.com/questions/18202532/can-a-single-stand-alone-literal-form-a-valid-json-document
[2] manual check & "JSON.generate only allows objects or arrays to be converted to JSON syntax." in http://ruby-doc.org/stdlib-2.0.0/libdoc/json/rdoc/JSON.html

Or since RFC 7159 supersedes RFC 4627, any legacy system will move toward it anyway.

jbenet · 2015-06-12T02:57:53Z

I re-ran the tests. something bad's going on.

at least this: https://travis-ci.org/ipfs/go-ipfs/jobs/66392490#L400

rht · 2015-06-12T13:37:33Z

I see, I likely didn't type the beginning slash when testing ipfs config Addresses.API /ip4/127.0.0.1/tcp/5001.

With this commit, all strings must be explicitly formatted as "string".

I suppose the options now are:

(Early version of this PR rht@f38c58d) Everything is json.
- It's tedious to make sure to surround strings with "'s.
- Breaks backward compatibility with env vars stored in dotfiles (...suppose this is not an issue).
- Every type is on equal footing.
- No need for "maybe use --json?"
Default to string. Keep the flags as they are currently, then just add '--number'. Where '--bool' and '--number' are actually '--json' in disguise.
- ipfs config foo 137 doesn't get parsed as number.
- much longer than option 3
Default to string. Use '--json' for everything else (object, array, number, bool, null).
- ipfs config foo 137 doesn't get parsed as number.
- Very roughly 75% of the time '--json' is required.
Default to json. If fails to unmarshall, then just set as string.
- Mixes valid string and malformed non-strings.
- No need for "maybe use --json?".

jbenet · 2015-06-13T01:06:16Z

(1.) normally, i'd like this to be the end state, but this breaks user space. :( not so sure.
(2.) -1 dont like this one.
(3.) I think i like this one. and deprecate --bool. (don't remove it yet, but later)
(4.) is a bit concerning, its the type of thing that backfires annoyingly. It may be ok we also have --json and --string just in case the auto-detect fails.

whyrusleeping · 2015-06-13T19:30:10Z

maybe we can be a little smart about things. things like: if strconv.Atoi doesnt return an error, we can assume its a number

jbenet · 2015-06-14T00:57:05Z

I'd prefer being simple and explicit. Smartness can introduce weird edge cases

rht · 2015-06-14T13:38:53Z

I think strconv.Atoi is a valid literal type inference. Will this be extended to Atob, Atof, json.Unmarshal (for list and object)... then fallback to string? If so this is basically (4.).

Default to string? (2, 3)
I have to amend one point in (3.): actually string is still the >50% most common value in the config (the reason why default to string is the current state in the first place). But other types are still common.
Default to json? (1, 4)
There is no need to "maintain '--bool' backward compatibility", since bools/numbers are automatically parsed. One can still specify '--bool'/'--number' but this is placebo.

The only breaking change in (1.) is the necessity of "--string".
It's just that you rarely see this in other configuration languages. But you also rarely see explicit type annotation like '--bool', '--number'.

(4.) by itself blurs the edge cases, i.e. it emulates the flexibility "smartness" of a human parser, but also its sloppiness. But this can still be fine because the config struct's are explicitly typed. And so this will always detect a malformed bool/int/float/array/object as a final check. This 2nd type check doesn't exist for user-specified values, however (and if this is hard to debug...). Also doesn't exist in dynamically typed languages.

There is actually option (5.): use https://github.com/vstakhov/libucl (as a superset of json) just for parsing then serialize to json.

There is no requirement of quotes for strings and keys, ...

It looks like strings can be inferred unambiguously and efficiently. But this is not a sufficient reason to integrate the whole library though (and also more formats to keep track of for compatibility: json, ucl, go types, other ipfs implementation types).

jbenet · 2015-06-30T18:44:30Z

@rht not sure here. i think we should make sure not to

break current user scripts (hence keep the --bool flag for now even if it doesnt do anything different)
make sure not to get into weird edge cases.
allow explicitness (make sure to keep --json which will only parse json or fail)

if you want to try (4.) then let's do it, but we'll need good tests to make sure various things get set correctly to what we expect.

rht · 2015-07-02T13:39:47Z

The only edge case with (4) is with user-specified values.
Everything else is unchanged.

The commands.Option object actually has a very similar use case.
In this case, the choice is (4) + value auto-cast (https://github.com/ipfs/go-ipfs/blob/master/commands/request.go#L224-L267), which makes sense because there won't be any user-specified command options.

jbenet added the backlog label Jun 11, 2015

rht and others added 2 commits June 11, 2015 21:41

Fix small typo

f2ef205

License: MIT Signed-off-by: rht <rhtbot@gmail.com>

rht force-pushed the cleanup-cli branch from 6e4bdbd to f38c58d Compare June 11, 2015 14:41

jbenet reviewed Jun 11, 2015
View reviewed changes

rht changed the title ~~Assume config value to be json-formatted~~ Assume config value to be json-formatted in cli Jun 12, 2015

rht closed this Jul 2, 2015

jbenet removed the backlog label Jul 2, 2015

rht mentioned this pull request Jul 2, 2015

Assert and convert config value into predefined struct #1440

Merged

rht deleted the cleanup-cli branch July 2, 2015 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assume config value to be json-formatted in cli #1355

Assume config value to be json-formatted in cli #1355

rht commented Jun 11, 2015

GitCop commented Jun 11, 2015

whyrusleeping commented Jun 11, 2015

jbenet Jun 11, 2015

wking Jun 11, 2015

rht Jun 12, 2015

jbenet Jun 12, 2015

rht Jun 12, 2015

jbenet commented Jun 12, 2015

rht commented Jun 12, 2015

jbenet commented Jun 13, 2015

whyrusleeping commented Jun 13, 2015

jbenet commented Jun 14, 2015

rht commented Jun 14, 2015

jbenet commented Jun 30, 2015

rht commented Jul 2, 2015

Assume config value to be json-formatted in cli #1355

Assume config value to be json-formatted in cli #1355

Conversation

rht commented Jun 11, 2015

GitCop commented Jun 11, 2015

whyrusleeping commented Jun 11, 2015

jbenet Jun 11, 2015

Choose a reason for hiding this comment

wking Jun 11, 2015

Choose a reason for hiding this comment

must now be

rht Jun 12, 2015

Choose a reason for hiding this comment

jbenet Jun 12, 2015

Choose a reason for hiding this comment

rht Jun 12, 2015

Choose a reason for hiding this comment

jbenet commented Jun 12, 2015

rht commented Jun 12, 2015

jbenet commented Jun 13, 2015

whyrusleeping commented Jun 13, 2015

jbenet commented Jun 14, 2015

rht commented Jun 14, 2015

jbenet commented Jun 30, 2015

rht commented Jul 2, 2015