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

feat:add strings option; re-implement file ignore #181

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

corntoole
Copy link
Contributor

This PR adds strings options type for passing in arrays of option values. The initial use-case for this feature is to pass multiple filepaths as options to IPFS file operations like add.

This PR also implements file-ignore by accepting a flag to use a local .gitignore file and/or an array of file paths to ignore similar to the example below:

$ tre
      1 .
      2 ├── another_dir
      3 │   ├── a.png
      4 │   └── b.png
      5 └── another_other_dir
      6     ├── bar_e.png
      7     ├── bar_f.png
      8     ├── bar_g.png
      9     ├── c.png
     10     ├── d.png
     11     ├── foo_a.png
     12     ├── foo_b.png
     13     └── foo_c.png
     14
     15 2 directories, 10 files
$ ipfs add -r --ignore "foo*.png" --ignore "bar*.png" another_other_dir/
added QmbFMke1KXqnYyBBWxB74N4c5SBnJMVAiMNRcGu6x1AwQH another_other_dir/c.png
added QmbFMke1KXqnYyBBWxB74N4c5SBnJMVAiMNRcGu6x1AwQH another_other_dir/d.png
added QmcLiRzrFw8SMTmN9umt2EJqzij8aCGGGVqcWAhZy7j2Sj another_other_dir
 0 B / ? [-------------------------------------------------------------------------=]

See ipfs#3643 and fission-suite/go-ipfs#2 for more context

@Stebalien Stebalien requested a review from dirkmc February 26, 2020 06:29
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on @corntoole

I left a couple of comments. Could we also add some tests for file ignore?

cli/parse.go Outdated Show resolved Hide resolved
cli/parse.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

This is looking good 👍

My only comment is that ignore-rules sounds like it does something similar to --ignore.
I take your point that ignore-file could be ambiguous, how about ignore-rules-path or --ignore-path?

opts.go Outdated Show resolved Hide resolved
cli/helptext_test.go Outdated Show resolved Hide resolved
@corntoole
Copy link
Contributor Author

Thanks for the review. I went with ignore-rules-path for the rules-file option.

cli/parse.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Stebalien please take a look

@dirkmc dirkmc requested a review from Stebalien March 12, 2020 16:07
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Some nits but this looks reasonable otherwise.

request.go Outdated
@@ -107,7 +107,7 @@ func (req *Request) convertOptions(root *Command) error {
}

kind := reflect.TypeOf(v).Kind()
if kind != opt.Type() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment here on why we don't need to convert string options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the change that excluded options with Type==Strings. I used an older PR as a starting point that had that exception, but I noticed that it would allow requests constructed with NewRequest(...) set non-array values. Would that comment still be necessary?

My understanding is that options with Type==String==reflect.String is not needed because v is already a string and does not need to be converted.

cli/parse.go Outdated Show resolved Hide resolved
cli/parse.go Outdated Show resolved Hide resolved
cli/parse.go Outdated Show resolved Hide resolved
@Stebalien
Copy link
Member

Ok, I've cut a release of go-ipfs-files. Could you squash, rebase, and update it (go get github.com/ipfs/go-ipfs-files@latest)?

- first step toward implementing file ignore

feat(file-ignore): implement file ignore

- add flags for providing a gitignore and/or list of files to ignore
- construct a filter to be passed to a SerialFile

feat(file-ignore): use go-ipfs-files fork; use renamed constructor

feat(file-ignore): test case w. ignore rules; refactor parseArgs

feat(file-ignore): ignore-rule opt is open-ended

- make ignore-rules options accept a path to a file
- add test case for ignore rules file

feat(file-ignore): help-text for variardic opts

- show StringsOption as variardic option in help-text
- add test case for variardic option

feat(file-ignore): rename rulesfile opt; fix typos

feat(file-ignore): temp replace of go-ipfs-files

- temporarily using go-ipfs-files fork until go-ipfs-files#26 is merged
- trying to get ci/cd builds working
- will revert before merging

feat(file-ignore): refactor cli/parse.go#setOpts

feat(file-ignore): cleanup parse.go; add test-case

feat(file-ignore): update ignore option copytext

feat(file-ignore): check opt against `optDef.Type`

- revert exclusion of options with `Strings` type,
so those option values are typechecked
- add command test cases

feat(file-ignore): add test-case w. hidden file

feat(file-ignore): build against go-ipfs-files@latest

feat(file-ignore): update go.mod/go.sum
@corntoole corntoole force-pushed the feat/add-strings-option branch from 56ed3c5 to 5fe469e Compare March 18, 2020 04:40
@corntoole
Copy link
Contributor Author

Ok, done. Thanks for the review

@Stebalien Stebalien merged commit 0722d72 into ipfs:master Mar 18, 2020
@Stebalien
Copy link
Member

Thanks for tackling this!

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