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

diff: reimplement interface and tests from scratch #3229

Merged
merged 35 commits into from Jan 30, 2020
Merged

diff: reimplement interface and tests from scratch #3229

merged 35 commits into from Jan 30, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 24, 2020

Close: #2982, #2998, #2999, #1893, #2372
Related: #770

  • Shows the difference between HEAD and the current workspace by default
  • Don't show files that stayed the same between both trees.
  • Remove --target option.
  • Checksums are only displayed when --checksum option is provided.
  • Format diff for machine readability when --show-json option is given.
  • Changes in output:
    • Remove file size information
    • Recurse directory outputs and display its cached information.
    • Group diff by state (i.e. "Added", "Deleted", "Modified")
  • Allow running dvc diff when there's no cache available.
  • Update completion scripts
  • Update documentation (diff: update docs according to the new patch dvc.org#953)

Questions:

  • Should we introduce a --recursive option to explicitly recurse directory outputs?
  • Should we support renamed files?

@ghost ghost requested a review from dmpetrov January 24, 2020 07:13
dvc/repo/diff.py Outdated Show resolved Hide resolved
tests/func/test_diff.py Outdated Show resolved Hide resolved
@efiop
Copy link
Contributor

efiop commented Jan 24, 2020

@MrOutis What is the point of these tests if they are all commented out and TODos?

dvc/repo/brancher.py Outdated Show resolved Hide resolved
@efiop
Copy link
Contributor

efiop commented Jan 24, 2020

As for the CLI, I was thinking on using the following as output:
Would it be OK to show checksums only when --checksums (new option) is specified?

I think the ticket was saying that we shouldn't show checksums by default. Wouldn't bother with introducing --checksums until someone asks. It should be present in json output though.

Suor
Suor previously requested changes Jan 24, 2020
Copy link
Contributor

@Suor Suor left a comment

Choose a reason for hiding this comment

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

Looks cool so far.

dvc/repo/diff.py Outdated Show resolved Hide resolved
dvc/repo/diff.py Outdated Show resolved Hide resolved
dvc/repo/diff.py Outdated Show resolved Hide resolved
dvc/repo/diff.py Outdated Show resolved Hide resolved
dvc/repo/diff.py Outdated Show resolved Hide resolved
dvc/command/diff.py Outdated Show resolved Hide resolved
@ghost ghost changed the title diff: reimplement interface and tests from scratch [WIP] diff: reimplement interface and tests from scratch Jan 24, 2020
* OSError -> FileNotFoundError
* Make `a_ref` optional
* Use absolute imports for unrelated packages
* Relpath: `str(output.path_info)` -> `str(output)`
* `a_ref` defaults to `HEAD` at the signature and parser level
dvc/command/diff.py Outdated Show resolved Hide resolved
tests/func/test_diff.py Outdated Show resolved Hide resolved
tests/func/test_diff.py Outdated Show resolved Hide resolved
dvc/repo/diff.py Outdated Show resolved Hide resolved
@ghost ghost changed the title [WIP] diff: reimplement interface and tests from scratch diff: reimplement interface and tests from scratch Jan 26, 2020
@ghost ghost requested review from efiop and Suor January 26, 2020 21:16
@ghost ghost dismissed Suor’s stale review January 26, 2020 21:16

@Suor, could you please take a look again? I did quite a lot of changes)

@ghost ghost changed the title diff: reimplement interface and tests from scratch [WIP] diff: reimplement interface and tests from scratch Jan 29, 2020
@ghost ghost changed the title [WIP] diff: reimplement interface and tests from scratch diff: reimplement interface and tests from scratch Jan 30, 2020
@ghost ghost requested a review from Suor January 30, 2020 00:57
Copy link
Member

@dmpetrov dmpetrov left a comment

Choose a reason for hiding this comment

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

I've done testing. New issues were created for features outside of the scope of this change: #3256, #3255. Great communication during the process!

LGTM 👍

@efiop
Copy link
Contributor

efiop commented Jan 30, 2020

@MrOutis please check the tests, looks like there are some dict ordering issues.

@ghost
Copy link
Author

ghost commented Jan 30, 2020

@efiop , doing it right now

Mr. Outis and others added 2 commits January 30, 2020 09:26
@ghost ghost dismissed Suor’s stale review January 30, 2020 16:01

@Suor, I changed the diff logic to use dicts

@efiop efiop merged commit fd1e6f4 into iterative:master Jan 30, 2020
@ghost ghost deleted the enhance-diff branch January 30, 2020 17:28
@jorgeorpinel
Copy link
Contributor

What's the fastest way to see if this is released or get notified when it is? I don't think it's included in 0.82.6, for example. Thanks

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.

diff: clean up output for changed files
4 participants