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

Docs for new command dvc check-ignore #1629

Merged
merged 21 commits into from
Aug 7, 2020

Conversation

karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Jul 26, 2020

Fixes #1628, Only a draft PR to explain how it works.

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

```

If the `--details` option is used, a series of lines are printed using this format:
`<source> <COLON> <linenum> <COLON> <pattern> <HT> <pathname>`
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 29, 2020

Choose a reason for hiding this comment

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

Suggested change
`<source> <COLON> <linenum> <COLON> <pattern> <HT> <pathname>`
`.dvcignore:<line number>:<pattern> <HT> <target path>`
  • Why <source>? It's always .dvcignore right? Or because of nested ones? If so maybe use <path/to/>.dvcignore
  • What's <HT>? Please replace with actual character.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karajan1001 actually I'm not sure what the answers are here, can you check it? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel,
Not always .dvcignore, might be data/.dvcignore as well. We didn't restrict .dvcignore to the root of the repo.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 7, 2020

Choose a reason for hiding this comment

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

OK so probably:

Suggested change
`<source> <COLON> <linenum> <COLON> <pattern> <HT> <pathname>`
`<path/to/.dvcignore>:<line_num>:<pattern> | <target_path>`

HT = horizontal tab, right?

UPDATE: Committed.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Thanks @karajan1001, great start! I've rewritten it to the point it's almost mergeable but there are some questions I left here and in iterative/dvc/pull/4282 that may affect the end result.

@shcheklein
Copy link
Member

@jorgeorpinel @karajan1001 the PR https://github.com/iterative/dvc/pull/42826 got merged today. what is the status of this? ready to merge as well?

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Awesome!! 😎 Please check some comments to address some minor changes + some technical stuff related to the engine.

@jorgeorpinel
Copy link
Contributor

I committed a bunch of my suggestions but there's still one or two questions pending from the old ones (plus Ivans comments) and the formatting needs fixing (sorry, may have been my bad) — we can just merge the Restyled PR though so that's not a huge deal.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

OK I finished the work here 🙂 Merging...

@jorgeorpinel jorgeorpinel requested a review from shcheklein August 7, 2020 03:13
@jorgeorpinel jorgeorpinel dismissed shcheklein’s stale review August 7, 2020 03:15

I'll take the liberty to merge this as-is since I addressed all your feedback. Another PR on this ref is coming soon so we can fix anything else in there...

@jorgeorpinel jorgeorpinel merged commit ddd3479 into iterative:master Aug 7, 2020
@karajan1001 karajan1001 deleted the fix_1628 branch August 7, 2020 03:17
according to the [`.dvcignore` file](/doc/user-guide/dvcignore) (if any). The
ones that are ignored indeed are printed back.

> Note that your shell may support path wildcards such as `dir/file*` and these
Copy link
Member

Choose a reason for hiding this comment

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

same - it's fine to keep it here, but that what I would expect anyways since multiple targets are supported. A bit not consistent since we don't make notes like this in other commands.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 7, 2020

Choose a reason for hiding this comment

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

Oh I see you found my note. Yeah any other commands where this note would be especially relevant? add/commit/remove? status/repro? fetch/pull/push? metrics/plots? (un)freeze?

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 7, 2020

Choose a reason for hiding this comment

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

I guess best to just remove it too...

Copy link
Contributor

Choose a reason for hiding this comment

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

Also removed in #1673


If DVC finds a `.dvcignore` file inside a dependency or output directory, it
raises an error. Ignoring files inside such directories should be handled from a
`.dvcignore` in higher levels of the project tree.

## Syntax

The same as for [`.gitignore`](https://git-scm.com/docs/gitignore).
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 that was useful. Also, we do quite a bit of changes not related directly to the command, @jorgeorpinel :)

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 7, 2020

Choose a reason for hiding this comment

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

Yes, sorry. But had to read the whole thing to find the best places to link to the command and did copy edits along the way.

This section was repetitive since the format (and templates) is already linked to from the bullets above. It was also an entire H2 section for 1-line — looked funny once rendered.

Should I restore it and move all the syntax/format and templates info here? (remove it from the bullet under How it works)

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Looks great 👍 Thanks 🙏 @karajan1001 @jorgeorpinel . Some comments to improve in the next revision.

Also, would be great eventually to use a bit simpler examples with some real case -e.g. ignore pycache - it applies to all dvcignore documents. I guess we have some tickets already.

jorgeorpinel added a commit that referenced this pull request Aug 7, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Aug 7, 2020
@jorgeorpinel
Copy link
Contributor

I guess we have some tickets already.

Just #519

shcheklein pushed a commit that referenced this pull request Aug 8, 2020
* cmd: remove unnecessary commas in get and import

* cmd: fix typo in add

* cmd: remote copy edits
per #1617 (comment)

* guide: .dvcignore copy edit

* cmd: init copy edits

* clarify about dirs in import -o

* cmd: review get -o desc

* dvcignore: updates to guide and check-ignore ref.
per #1629 (review)
et al.

* cmd: update check-ignore -n
per iterative/dvc#4323 (comment)

* cmd: fix get.import -o descriptions
per #1673 (review)
and #1673 (review)

* cmd: copy edits to remote add/modify
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.

cmd: document check-ignore
3 participants