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

Add strings option #4012

Closed
wants to merge 2 commits into from
Closed

Add strings option #4012

wants to merge 2 commits into from

Conversation

crackcomm
Copy link

@crackcomm crackcomm commented Jun 27, 2017

Command strings array option.

It works like that:

$ ipfs -i node_modules -i .git -i yarn.lock -r .

It's required to get #3643 done for above to work – ignoring paths.

License: MIT
Signed-off-by: Łukasz Kurowski <crackcomm@gmail.com>
@crackcomm crackcomm changed the title cmd options: strings array Add strings option Jun 27, 2017
@whyrusleeping
Copy link
Member

This feature sounds good to me, makes sense to have and i can definitely see it being useful. I'd like to see some tests around this.

Also, we will have to coordinate adding this feature with @keks refactor work. Shouldnt be a big deal, but definitely need to account for it.

@whyrusleeping whyrusleeping requested review from keks, kevina and Kubuxu June 27, 2017 04:57
@kevina
Copy link
Contributor

kevina commented Jun 28, 2017

@crackcomm the tests failure in ci/circleci look related to your changes

License: MIT
Signed-off-by: Łukasz Kurowski <crackcomm@gmail.com>
@crackcomm
Copy link
Author

@kevina passed now

@whyrusleeping
Copy link
Member

@crackcomm could you add a test or two to commands/cli/parse_test.go for this new flag type?

@keks
Copy link
Contributor

keks commented Jun 29, 2017

The feature and the code LGTM. Since at this point there are zero tests, wouldn't it be better to make this change on top of #3856? If we keep changing commands that one will never get merged.

@crackcomm
Copy link
Author

@keks feel free to do so.

@crackcomm
Copy link
Author

btw. there are some tests in #4013

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

@keks do you think you could introduce something like this after the commands lib rework is merged?

(cleaning out my Review queue).

@keks
Copy link
Contributor

keks commented Oct 16, 2017

Sure, I'll do that

@hsanjuan
Copy link
Contributor

This should be resolved in go-ipfs-cmds if anywhere. So closing this one.

@hsanjuan hsanjuan closed this Mar 23, 2020
@Stebalien
Copy link
Member

Fixed a few days ago, actually...

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.

7 participants