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 #1033 user-guide: add more links to the .dvcignore guide from other docs #1328

Closed
wants to merge 46 commits into from
Closed

Conversation

sarthakforwet
Copy link
Contributor

@sarthakforwet sarthakforwet commented May 21, 2020

Added references to .dvcignore from other docs and also listed an example in docs of dvc move.

Closes #1033

@jorgeorpinel
Copy link
Contributor

Hi @sarthakforwet I'll review this ASAP. Please notice that #1329 was open automatically because the formatting of your source code doesn't follow the guidelines explained in https://dvc.org/doc/user-guide/contributing/docs (see dev environment and doc style sections). Please address this meanwhile.

@shcheklein
Copy link
Member

@sarthakforwet I see a lot of changes! thanks 🙏But from what I get by briefly taking a look at them - it's not exactly what expected in the ticket, and I'm not sure that they are correct to be honest. I know that the ticker itself is not precisely defined, thus what I suggest before we proceed here, let's in the ticket itself write things we want to mention, use case we have in mind for people to be aware, write simple scripts to show those use case. So that we are on the same page + scripts. Closing this for now, we'll reopen it when we have a plan of changes.

@shcheklein shcheklein closed this May 21, 2020
@sarthakforwet
Copy link
Contributor Author

sarthakforwet commented May 22, 2020

Hey, what ticket means? Can you please elaborate it?
Also are you saying to write scripts regarding the changes i have mentioned to justify them?

@shcheklein
Copy link
Member

Hey, what ticket means?

not sure I got the question

Also are you saying to write scripts regarding the changes i have mentioned to justify them?

no, I mean simple scripts (show cases) in the ticket so that we clearly understand what use cases are we writing about:

vim .dvcignore
// add .DS_Store there
git commit .dvcignore -m "dvc: ignore Mac OS files in outputs and input"

something like this should be enough ^^

let's say we add a mention of .dvcignore to some DVC page - we should have a clear scenario in mind.

@jorgeorpinel
Copy link
Contributor

@sarthakforwet hi! For future reference, please link the PR to the original issue in following attempts. I've updated the description here for your ref.

@jorgeorpinel
Copy link
Contributor

what I suggest before we proceed here, let's in the ticket itself write things we want to mention, use case we have in mind for people to be aware, write simple scripts to show those use case. So that we are on the same page + scripts

what ticket means?

@sarthakforwet Ivan meant the ticket that this PR attempts to close, #1033

are you saying to write scripts regarding the changes i have mentioned to justify them?

To be extra clear, Ivan is pointing out that the original ticket or issue (#1033) is kind of vague and open to interpretation so that's part of the reason why your changes here were not what we expected. I think I need to work on that issue a little more for it to really be a good-first-issue...

@sarthakforwet
Copy link
Contributor Author

I think changes like below describes relationship of .dvcignore with add and move and these kind of solves some of the checkboxes listed at #1033 ?

Changes_Add

dvc_move

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.

Hey @sarthakforwet I left some specific feedback as agreed although I think you may be working on another issue instead? Not sure sorry lost track. If you get back to this, please notice the original issue has more specific checkboxes so you can open a PR to address just one of them and we can go one by one. Some of the text in this PR could be reused possibly.

Comment on lines +247 to +248
To untrack a file or directory just add [patterns](https://git-scm.com/docs/gitignore) (corresponding to the location of file or directory) under `.dvcignore` file.<br>
See [.dvcignore](docs/user-guide/.dvcignore) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a dvcignore ref in this doc is def. needed but here you're just appending at the end of the examples. Please read the page (rendered in the website), check the structure, and find a better place to add this ref.

Adding an add example specific to dvcignore would also be nice.

Comment on lines 72 to +74

Note that [patterns](https://git-scm.com/docs/gitignore) listed in `.dvcignore` are not updated as a result of `dvc commit` as they are not currently tracked by DVC. See [.dvcignore](docs/user-guide/.dvcignore) for more details.

Copy link
Contributor

Choose a reason for hiding this comment

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

This note was in a more appropriate place. The paragraph just needed formatting (see docs contrib guide) and also I would not link patterns to the Git website in any of these docs to avoid confusions.

Copy link
Contributor

Choose a reason for hiding this comment

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

The link href is incorrect though, it's missing a / in the beginning, please run the app locally so you can see and try your changes. See docs contrib guide.

Comment on lines +74 to +84
Note that when we try to use `dvc move` over a file whose pattern matches one of the patterns listed in `.dvcignore`, it would raise an error because that DVC-file was not tracked by DVC.

```dvc
$ dvc add data.csv
$ echo data.* >> .dvcignore
$ dvc move data.csv other.csv
ERROR: failed to move 'data.csv' -> 'other.csv' - Unable to find DVC-file with output 'data.csv'

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!
```
See [.dvcignore](docs/user-guide/.dvcignore) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

This also looks good to me, maybe just put it under an H2 heading such as ## Dvcignore effects. Same in other docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it's probably best without header and also without example. Just a very short note should do the trick here. We just want users to be aware of .dvcignore, not to explain it in every command ref.

Comment on lines +59 to +60
Note that when you do `dvc pull` then the missing files whose corresponding DVC-files matches with the DVC-files in remote storage will be downloaded. But if a DVC-file is listed under `.dvcignore` then its corresponding file won't be downloaded.<br>
See [.dvcignore](docs/user-guide/.dvcignore) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Misisng a space and please don't add <br>s, just regular break lines

Suggested change
Note that when you do `dvc pull` then the missing files whose corresponding DVC-files matches with the DVC-files in remote storage will be downloaded. But if a DVC-file is listed under `.dvcignore` then its corresponding file won't be downloaded.<br>
See [.dvcignore](docs/user-guide/.dvcignore) for more details.
DVC might remove ignored files (files listed in `.dvcignore`) upon `dvc run` or `dvc repro`. If they are not produced by a pipeline stage, they can be deleted permanently. See [.dvcignore](docs/user-guide/.dvcignore) for more details.

It also needs formatting (max 80 char lines).

@@ -70,6 +70,8 @@ backward from the target [stage files](/doc/command-reference/run), through the
corresponding [pipelines](/doc/command-reference/pipeline), to find data files
to push.

Note that as `dvc push` uploads tracked files and directories to remote storage, it won't upload files and directories listed under `.dvcignore`.<br> See [.dvcignore](docs/user-guide/.dvcignore) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is the case? Even if the data was tracked before adding it to .dvcignore? If so this note is OK, just again: no <br> and needs formatting.

@sarthakforwet
Copy link
Contributor Author

@jorgeorpinel Thank you for your review. I am correcting the issues and will soon submit PR's One by One Sequentially with proper formatting.

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.

guide: add more links to the .dvcignore guide from other docs
3 participants