Skip to content

diff: update docs according to the new patch#953

Merged
shcheklein merged 7 commits into
masterfrom
update-diff
Feb 24, 2020
Merged

diff: update docs according to the new patch#953
shcheklein merged 7 commits into
masterfrom
update-diff

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Jan 28, 2020

@shcheklein shcheklein temporarily deployed to dvc-landing-update-diff-nf8zlg January 28, 2020 05:06 Inactive
@ghost ghost requested a review from jorgeorpinel January 28, 2020 05:07
@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel @MrOutis I'll take this over and get it done.

@jorgeorpinel please, focus on the existing backlog.

@shcheklein shcheklein requested review from shcheklein and removed request for jorgeorpinel January 28, 2020 07:04
@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Jan 31, 2020

Thanks Ivan. I just want to post the link to the original PR: treeverse/dvc/pull/3229

Also, there will probably be some conflicts here or in #933, depending on which one is merged first.

p.s. these changes don't seem to be released yet in latest the version, 0.82.6 – may want to keep that in mind before merging

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Someone on Discord just noticed this doc is outdated. Should we bump the priority up a little? I can take it if needed.

@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel I'm on it. let's get api done (i'll try to review it soon), and then switch to updating get started and other new stuff.

@shcheklein
Copy link
Copy Markdown
Contributor

The next iteration will be done after we get more clarify on how it should actually behave in certain cases - see treeverse/dvc#3386 and treeverse/dvc#3385 .

Also, I would like to reconsider the way we do examples (with all these expandable sections). It's time to utilize Katacoda or other online services. For each example there should be a link to go and try online (which includes the instruction on how it was setup in the first place). This way we can decouple and simplify examples we write.

@shcheklein shcheklein removed their request for review February 24, 2020 00:19
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-update-diff-tiyi2w February 24, 2020 02:23 Inactive
Copy link
Copy Markdown
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.

Full review of the changes so far below. Thanks!

Comment thread public/static/docs/command-reference/diff.md
Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
of the output produced. See the details and example below.

`dvc diff` does not have an effect when the repository is not tracked by Git,
for example when `dvc init` was used with the `--no-scm` option.
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.

dvc diff does not have an effect... when dvc init was used with the --no-scm option

Somewhat unrelated but actually, I checked and this isn't correct. You can create a git repo, then a dvc init --no-scm in it, dvc add a data file, commit (A); change and dvc add the data file again, commit (B), and dvc diff HEAD^ will work.

Is this buggy behavior? If not, we just need to rewrite the paragraph above to something more correct.

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 feels more or less correct to me - the main thing here is when the repository is not tracked by Git. In your example it is tracked by Git, right? unless I'm missing something.

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.

Yes it's tracked by Git, but I initialized the DVC project with dvc init --no-scm. The paragraph above reads ...`dvc diff` does not have an effect... for example when `dvc init` was used with the `--no-scm` option but it does have an effect, as long as there is a Git repo underneath. Maybe just remove the part about --no-scm in the paragraph?

This comment was marked as off-topic.

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's fine, I still don't see a problem. It's intuitively clear what does it mean, even if implementation is not 100% correct and allows to have a mix of Git and --no-scm.

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.

Hmmm OK but just removing that last incorrect statement wouldn't hurt either.

Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
@jorgeorpinel

This comment has been minimized.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Oh and from treeverse/dvc#3229 the only thing that's not reflected in the cmd ref is "Allow running dvc diff when there's no cache available." but not sure it's worth mentioning in docs.

@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel

We could do that in a separate PR @shcheklein ?

yes, it'll be a separate PR for sure! We need to merge it sooner.

But it also sounds like a separate and larger task.

it's not that large, we just need to start doing this step by step. I'll create a separate section on Katacoda to define structure and create examples for dvc diff - I think it takes half of the day first time, but then it'll be even easier to reuse them then writing a lot of stuff over and over again.

@shcheklein

This comment has been minimized.

@shcheklein shcheklein temporarily deployed to dvc-landing-update-diff-tiyi2w February 24, 2020 03:06 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-update-diff-tiyi2w February 24, 2020 03:08 Inactive
@shcheklein

This comment has been minimized.

@shcheklein
Copy link
Copy Markdown
Contributor

For the record, created a few tickets regarding the cache-less behavior. To be honest both look like a bug for me - treeverse/dvc#3392 and treeverse/dvc#3391

Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md Outdated
Comment thread public/static/docs/command-reference/diff.md
of the output produced. See the details and example below.

`dvc diff` does not have an effect when the repository is not tracked by Git,
for example when `dvc init` was used with the `--no-scm` option.
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.

Yes it's tracked by Git, but I initialized the DVC project with dvc init --no-scm. The paragraph above reads ...`dvc diff` does not have an effect... for example when `dvc init` was used with the `--no-scm` option but it does have an effect, as long as there is a Git repo underneath. Maybe just remove the part about --no-scm in the paragraph?

Copy link
Copy Markdown
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.

No strong opinion about these last few details (above ⬆️).

Comment thread public/static/docs/command-reference/diff.md
@shcheklein shcheklein temporarily deployed to dvc-landing-update-diff-tiyi2w February 24, 2020 07:34 Inactive
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.

2 participants