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

Fix config option #214

Closed
wants to merge 2 commits into from
Closed

Conversation

chriscool
Copy link
Contributor

The first patch in this PR makes "ipfs -c=dir/.go-ipfs init" work.
The second patch adds some tests to catch any regression.

When running something like:

$ ipfs -c="file/path/.go-ipfs" init

options must be parsed before looking up a sub-command.

Otherwise -c="file/path/.go-ipfs" will be the key for
the sub-command lookup and it will not work.

License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
License: MIT
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
@whyrusleeping
Copy link
Member

I recall making that change to commander in the past and it had some odd consequences...
I beleive @jbenet was weirded out that it required the '-c' option to be specified before any subcommand

@jbenet
Copy link
Member

jbenet commented Oct 27, 2014

@chriscool we shouldn't modify anything under Godeps directly, as that's vendored code (Go's approach to package management). We should change the source repos and then re-vendor. Otherwise we'll get horribly off sync with upstreams.

We can modify my fork of commander here: https://github.com/jbenet/commander but, mappum's commands PR (#196) will move us into our own commands/ package soon. (i.e. the flag will change then -- for now use the ENV var)

Also, -c may conflict with child flags. I think we should just use --config as this is a strong enough (+ command wide) flag to warrant typing the extra characters.

(feel free to rebase the branch)


The test LGTM!

@chriscool
Copy link
Contributor Author

Ok, this can wait until PR #196 and our own commands/ package.

About conflicting with child flags, I think "ipfs" command flags should be independent from child command flags.

For example with git ou can say:

git -c color.ui=false grep -c ok

where "-c color.ui=false" is a "git" command flag and the other "-c" is a "git grep" command flag.

By the way it could be useful for ipfs to have a way to change config values just for one command, like the above "-c name=value" "git" command flag.

@jbenet
Copy link
Member

jbenet commented Oct 27, 2014

I think "ipfs" command flags should be independent from child command flags.

Yeah I was leaning towards this as well at first. (i.e. (<cmd1> [<flags1> ])+ [<args>])), which is logically better. But from a UX standpoint, many tools end up making flags usable all over, e.g cmd1 cmd2 cmd3 arg -flag3 -flag2, which (when coupled with non-conflicting flag names) makes life easier for the user.

@chriscool do you have more examples of clashing flag names in git? or is this a rare occurrence?

@chriscool
Copy link
Contributor Author

It is a rare occurence because there are very few one letter flags for the "git" command: -c, -C and -p.
See: "$ git help"

By the way it looks like docker uses the same docker <option>... <cmd> <option>... <args>... pattern as git, and it looks like there could more often be conflicting options.
See: "$ docker help" and "$ docker help run"

@whyrusleeping
Copy link
Member

👍

@whyrusleeping
Copy link
Member

@chriscool I really gotta say that these tests youre writing are awesome

@jbenet
Copy link
Member

jbenet commented Oct 29, 2014

Seconded, these are so useful!

@chriscool
Copy link
Contributor Author

Thanks guys! I am happy that you like them.

I will write more of them, especially this week end.
In the end though, it should be like when developing Git these days, everyone who makes some significant changes or fixes a significant bug in the CLI should add some tests.

Btw I am often lurking at the IRC archive and I like what you are doing very much. It feels great to see how fast the project is moving! It reminds me when I started working on Git in 2006 :-)

@whyrusleeping
Copy link
Member

@chriscool is this PR still something that could be merged? or should we just close it?

@whyrusleeping
Copy link
Member

closing due to inactivity, if needed, reopen

@chriscool
Copy link
Contributor Author

Yeah, sorry for not responding. I think it was too old and could not be merged anymore.

@whyrusleeping
Copy link
Member

no worries, just working on cleaning up our stale PRs.

@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
ariescodescream pushed a commit to ariescodescream/go-ipfs that referenced this pull request Oct 23, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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.

3 participants