Skip to content

[WIP] dvcignore: update description#494

Merged
shcheklein merged 2 commits into
treeverse:masterfrom
pared:update_dvcignore
Jul 31, 2019
Merged

[WIP] dvcignore: update description#494
shcheklein merged 2 commits into
treeverse:masterfrom
pared:update_dvcignore

Conversation

@pared
Copy link
Copy Markdown
Contributor

@pared pared commented Jul 22, 2019

IMPORTANT NOTES (please read, then delete):

  • Have you followed the guidelines in our
    Contributing Documentation?

  • The PR title should start with "Fix #bugnum: " (if applicable), followed by a
    clear one-line present-tense summary of the changes introduced in the PR. For
    example: "Fix #bugnum: Introduce the first version of the collection editor.".

  • Please make sure to mention "Fix #bugnum" (if applicable) somewhere in the
    description of the PR. This enables Github to link the PR to the corresponding
    bug.

Fixes #485

@pared pared requested a review from shcheklein July 22, 2019 10:02
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
- `.dvcignore` is no longer applied only on operations collecting DVC-files.
Each time DVC traverses directory, dvcignore files up to project root tree
are respected.
- ignoredd files will not be cached and preserved in any way, they will
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ignored

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it needs more details on what does "preserved" means. It's not clear. For example, when we remove outputs do we skip ignored files or remove them as well? It worth describing and mentioning these edge cases. And give examples below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree, I think I should remve the preserved part. The most important thinkg about this point is that ignored files will not be cached. Ill inkclude removing edge case in new point.

Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
be non-existent for DVC. Its worth to remember that, especially when ignoring
files inside DVC-handled directories.
- ignoring files inside directory dependencies under DVC control means that
this dependency has changed, as if ignored files were deleted, so commiting
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it about dependency or outputs as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Outputs too.

Comment thread static/docs/user-guide/dvcignore.md Outdated
- ignoring files inside directory dependencies under DVC control means that
this dependency has changed, as if ignored files were deleted, so commiting
"new" version of dependency might be required.
- If DVC will stumble upon `.dvcignore` file inside dependency directory, it
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it about dependency or outputs as well?

Copy link
Copy Markdown
Contributor

@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.

Good stuff!

Some changes are needed (see comments) + we need to update examples probably.

@pared pared force-pushed the update_dvcignore branch from 98fd011 to 20a5fba Compare July 24, 2019 12:24
@pared pared changed the title dvcignore: update description [WIP] dvcignore: update description Jul 25, 2019
@pared pared force-pushed the update_dvcignore branch from 70a38e8 to 3ee17dc Compare July 25, 2019 09:18
Comment thread static/docs/user-guide/dvcignore.md Outdated
- You need to create `.dvcignore` file.
- Populate it with [patterns](https://git-scm.com/docs/gitignore) that you would
like to ignore.
like to ignore
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why this change?

Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
is advised to use it in cases described in the first paragraph, when amount of
files in tree of repository directory causes performance issues.

- Ignored files will not be cached, they will be non-existent for DVC. It's
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cached -> taken under DVC control, or saved. cached is an internal term more or less. It's always better to explain stuff in terms that come from the end-user world, not from the internal implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Lets meet halfway: saved in cache

Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Copy link
Copy Markdown
Contributor

@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.

Good stuff. Check my comments and would be great to put more examples?

@shcheklein shcheklein requested a review from jorgeorpinel July 26, 2019 02:14
@pared
Copy link
Copy Markdown
Contributor Author

pared commented Jul 26, 2019

Good stuff. Check my comments and would be great to put more examples?

Yes, Ill introduce them.

Comment thread src/Documentation/glossary.js Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md
Comment thread static/docs/user-guide/dvcignore.md Outdated
Copy link
Copy Markdown
Contributor

@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.

Good stuff, still some changes are required.

@pared pared force-pushed the update_dvcignore branch from 918b374 to e845c1e Compare July 29, 2019 13:38
Comment thread static/docs/user-guide/dvcignore.md
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread src/Documentation/glossary.js Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md
Comment thread static/docs/user-guide/dvcignore.md Outdated
@@ -18,26 +19,126 @@ provide similar functionality as `.gitignore` files provide for `git`.
- Each line should contain only one pattern;
- During execution of commands that traverse directories, DVC will ignore
matching paths;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

traverse directories - it's not only about dirs, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ignore is not only about dirs, but it works only during traversing of dirs :)

Comment thread static/docs/user-guide/dvcignore.md Outdated
@@ -18,26 +19,126 @@ provide similar functionality as `.gitignore` files provide for `git`.
- Each line should contain only one pattern;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please, use . instead of ; at the end of each item

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, but I did it to match changes introduced for unification of bullet lists. Isn't it the way we are handling bullet lists now?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nope, we're avoiding ; moving forward (: Refer to #491 (comment)

Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Comment thread static/docs/user-guide/dvcignore.md Outdated
Copy link
Copy Markdown
Contributor

@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.

almost there :) just a few comments, typos and small improvements.

@pared pared force-pushed the update_dvcignore branch from 3bea44d to 6acf73d Compare July 31, 2019 12:01
@pared pared requested a review from shcheklein July 31, 2019 12:12
@shcheklein
Copy link
Copy Markdown
Contributor

thanks!

@shcheklein shcheklein merged commit 6938515 into treeverse:master Jul 31, 2019
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.

update dvcignore docs

3 participants