Skip to content

diff command: output in markdown format with hash#4435

Merged
efiop merged 3 commits into
treeverse:masterfrom
PuneethaPai:diff_md_hash
Aug 23, 2020
Merged

diff command: output in markdown format with hash#4435
efiop merged 3 commits into
treeverse:masterfrom
PuneethaPai:diff_md_hash

Conversation

@PuneethaPai
Copy link
Copy Markdown
Contributor

@PuneethaPai PuneethaPai commented Aug 20, 2020

Fixes #4313

PS: there may be scope for refactoring. Inputs are welcome ❗ 😄

@PuneethaPai
Copy link
Copy Markdown
Contributor Author

Is sorting file paths based on name necessary?

Because looking at code, only for --show-md we are sorting files.

For default and show-json we are not sorting files. But doc string says files will be sorted for better readability here

@pared pared self-requested a review August 20, 2020 21:27
@PuneethaPai PuneethaPai changed the title Diff md hash diff command: output in markdown format with hash Aug 21, 2020
Comment thread tests/unit/command/test_diff.py Outdated
Comment thread tests/unit/command/test_diff.py Outdated
Copy link
Copy Markdown
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

One cosmetic change

@PuneethaPai
Copy link
Copy Markdown
Contributor Author

Is sorting file paths based on name necessary?
Because looking at code, only for --show-md we are sorting files.
For default and show-json we are not sorting files. But doc string says files will be sorted for better readability here

Any inputs on this. I still feel json and default formats of dvc diff is not sorting file names ❗
Only sorting in markdown seems inconsistent.. 😟

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Aug 23, 2020

@PuneethaPai Thank you for your patience! Good catch! Indeed, for json it doesn't really matter, since it is meant for programmatic use anyways. But default output would indeed benefit from sorting. Added checkbox in #2982

Thank you so much! 🙏

@efiop efiop merged commit 05c1e76 into treeverse:master Aug 23, 2020
@PuneethaPai PuneethaPai deleted the diff_md_hash branch August 24, 2020 04:59
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.

dvc diff --show-hash --show-md <commit> shows only file paths, doesn't show md5 hash

3 participants