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/metics diff: update argument descriptions, et al. #3244

Merged
merged 14 commits into from
Feb 3, 2020
Merged

diff/metics diff: update argument descriptions, et al. #3244

merged 14 commits into from
Feb 3, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Jan 27, 2020

to match iterative/dvc.org#933

  • ❗ Have you followed the guidelines in the Contributing to DVC list?

  • 📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

  • ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

dvc/command/diff.py Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel marked this pull request as ready for review January 29, 2020 06:39
@jorgeorpinel jorgeorpinel changed the title diff/metics diff: update argument descriptions diff/metics diff: update argument descriptions, et al. Jan 29, 2020
setup.py Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Jan 30, 2020

@jorgeorpinel , looks like you've change the old diff implementation, I will include your changes in my PR.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@jorgeorpinel , the new implementation of dvc diff got merged, please update your PR accordingly

@jorgeorpinel
Copy link
Contributor Author

looks like you've change the old diff implementation

@MrOutis I changed just some of the help output stings, but I'm merging with master now and (mostly) preserved your text. Please check again 🙂

@jorgeorpinel jorgeorpinel requested a review from a user January 30, 2020 18:27
jorgeorpinel added a commit to efiop/dvc.org that referenced this pull request Jan 30, 2020
Copy link
Contributor Author

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

Self review below (1 comment). Also:

  • Update autocomplete scripts per diff/metrics diff output updates.

desc="Computing hashes (only done once)",
desc="Computing file checksum(s) (only done once)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably revert per iterative/dvc.org#933 (review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I reverted this and applied "hash" terminology as much as possible to the whole file, as well as a few others in 5019fe1. If this commit is acceptable, I can update all the other instances (there's less than 70, in 19 files.
Or I can extract further changes on this to a separate issue/PR. Cc @MrOutis @efiop

dvc/command/diff.py Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

cool, @jorgeorpinel 👍

just left a comment where I didn't understand your change, but I guess it is not that crucial to stop this from being merged

@jorgeorpinel jorgeorpinel requested review from a user and efiop January 31, 2020 00:26
dvc/command/diff.py Outdated Show resolved Hide resolved
@@ -134,17 +133,17 @@ def add_parser(subparsers, parent_parser):
diff_parser.add_argument(
"a_ref",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to rename ref -> rev in args too. Can do after this PR is merged.

@@ -11,7 +11,7 @@


def _generate_version(base_version):
"""Generate a version with information about the git repository"""
"""Generate a version with information about the Git repository."""
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, not a big fan of such changes in the code. It only pollutes the history and doesn't provide any value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm used to making copy edits on docs all the time so this is kind of automatic for me but whatever policies you tell me for the core repo I'll abide to the best of my ability. Personally I don't understand the problem of history pollution but I guess it complicates your release management? Anyway, I'll follow your lead on these things of course.

@@ -156,7 +155,7 @@ def add_parser(subparsers, parent_parser):
)
diff_parser.add_argument(
"--checksums",
Copy link
Contributor

Choose a reason for hiding this comment

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

Will need to rename checksum -> hash in the whole code base. Need to do that in a sync way to not introduce more confusion for developers and users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record, this question has been extracted to #3311

@efiop
Copy link
Contributor

efiop commented Feb 3, 2020

Alright, I'm not happy about the mix of things here, but let's merge and move on. Please try to not dilute the focus of your PRs. Create new ones if you decide to change something else that is not related. That way it is easier to review and merge, so we could all move faster.

@efiop efiop merged commit de1f67a into iterative:master Feb 3, 2020
shcheklein pushed a commit to iterative/dvc.org that referenced this pull request Feb 13, 2020
* add docs for dvc metrics diff

* nav: add `metrics diff` to sidebar

* cmd ref: typos in `metrics diff`

* cmd ref: rewrite `metrics diff` ref and
and review related concepts throughout docs e.g. "Git reference", "working tree"

* cmd ref: update descs, review options, link all metrics subcmds
addresses #933 (review)
as well as #933 (review)
and #933 (review)

* cmd ref: update cmd argument descriptions for `diff` and `metics diff`

* metrics diff: big terminology review around the intro of this new command
per #933 (review) et al.

* term: review usage of "hash", "commit hash", "SHA", and "MD5"
per #933 (review)

* term: rewrite definition of "workspace"
per #933 (review)

* cmd ref: change link from `metrics diff` options to `metrics show`
per #933 (comment)

* cmd ref: update example in `dvc metrics diff` and similar ones
per #933 (review)

* cmd ref: simplify dvc gc -a option
per #933 (review)
and #933 (review)

* cmd ref: use "reference" more than "revision" in diff
per #933 (review)

* cmd ref: link term "revision" in diff and `metrics diff`
also per #933 (review)

* term: put Git ref exapmles before term and link
per #933 (review)

* cmd ref: friendlier explanation of "tip of default branch"
per #933 (review)

* cmd ref: use tag name instead of term "the revision"
per #933 (review)

* term: revert some "revision"->"reference" changes, and related simplifications
per #933 (review)

* cmd ref: review desc. of `-a` options throughout refs

* cmd ref: update diff params
per iterative/dvc/pull/3244

* cmd ref: update notes around moving/static Git refs in import and update
per #933 (review)

* revert workspace glossary entry
per #933 (review)

* tutorial: use full name of Deep Dive Tutorial in title and links
per #933 (review)

* user-guide: undo change on "binary" literal for analytics example
per #933 (review)

* use-cases: avoid term "revision" in data-registries
per #933 (review)

* term: revert "hash"->"checksum" in this PR
per #933 (comment)

* cmd ref: "revision"->"commit" in get ref
per #933 (review)

* cmd ref: use correct tag names in checkout examples
and double check they still work
per #933 (review)

* diff: remove backquotes adound "HEAD" same as in core repo
per iterative/dvc#3244 (review)

* cmd ref: don't use link to git reference doc
per #933 (comment)

* cmd ref: don't use term "revision" in diff, prefer "commit"
per #933 (review)

* cmd ref: no need for word "specific" (or "SHA") in get/import
per #933 (review)

* cmd ref: update "project"->"workspace" term and example intros in `dvc install`
per #933 (review)

* docs: 2 misc updates
per #933 (review)
and #933 (review)

* tutorials: update model->"data or model"
per #933 (review) et al.

* cmd ref: fixed link to `metrics diff` and updated mention of it in `metrics show`
per #933 (review)
and #933 (review)
and #933 (review)

* get-started: typo in pipelines chapter

* cmd ref: rewrite paragraph about fixed revision import stages in `update`
per #933 (review)

* cmd ref: rewrite p about `repro` rewriting artifacts in cache
per #933 (review)

* cmd ref: rephrase and split p about what to compare and about targets in `status`
per #933 (review)

* cmd ref: reorg last part of repro desc

* cmd ref: restore `git tag` sample output in checkout examples
per #933 (review)
and #961 (comment)

* cmd ref: small rewording around tag names

* cmd ref: `metrics diff` and `diff` intro updates
per #933 (review)
and #933 (review)

* term: use "version" instead of "revision" in `import` cmd ref
per #933 (comment)

* cmd ref: updates to `metrics diff` (and `diff`) descriptions
per https://dvc.org/doc/command-reference/status
and #933 (review)
and #933 (review)

* cmd ref: change "ref" -> "rev" per iterative/dvc/pull/3299
and #933 (review)

* cmd ref: "revision"->"version" in a couple more docs
per #933 (review)

* cmd ref: simplify note about `metrics diff` in `metrics show`
per #933 (review)

* cmd ref: use descriptive exampe-get-started repo tags in `get` examples
per #933 (review)

* term: "commit hash"->"commit SHA hash" to match #962 but
may change the decision in that other PR.

* cmd ref: improve -a adn -c option descs
per #933 (review)

* cmd ref: remove p about --targets option in metrics diff
per #933 (review)

* cmd ref: rewrite pa bout fixed revisions/re-importing in `update`
per #933 (review)

* tutorials: use and instead of data "or" models in versioning tut
per #933 (review)
and #933 (review)

* user-guide: restore bullet about `git` in analytics
per #933 (review)

* you don't usually merge tags
per #933 (review)

* term: don't use "version of repo/project" when referring to commits
per #933 (comment)

* cmd ref: simlpify note about metrics diff in metrics show
per #933 (review)

* cmd ref: and->and/or in checkout sample of versioning tut
per #933 (review)

* user-guide: updated analytics details
per #933 (review)

* cmd ref: restore simpler wording about status -aT desc
per #933 (review)

* cmd ref: correct (again) the short desc for diff and metrics diff
per #933 (comment)

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
shcheklein pushed a commit to iterative/dvc.org that referenced this pull request Feb 13, 2020
…ext) (#962)

* add docs for dvc metrics diff

* nav: add `metrics diff` to sidebar

* cmd ref: typos in `metrics diff`

* cmd ref: rewrite `metrics diff` ref and
and review related concepts throughout docs e.g. "Git reference", "working tree"

* cmd ref: update descs, review options, link all metrics subcmds
addresses #933 (review)
as well as #933 (review)
and #933 (review)

* cmd ref: update cmd argument descriptions for `diff` and `metics diff`

* metrics diff: big terminology review around the intro of this new command
per #933 (review) et al.

* term: review usage of "hash", "commit hash", "SHA", and "MD5"
per #933 (review)

* term: rewrite definition of "workspace"
per #933 (review)

* cmd ref: change link from `metrics diff` options to `metrics show`
per #933 (comment)

* cmd ref: update example in `dvc metrics diff` and similar ones
per #933 (review)

* cmd ref: simplify dvc gc -a option
per #933 (review)
and #933 (review)

* cmd ref: use "reference" more than "revision" in diff
per #933 (review)

* cmd ref: link term "revision" in diff and `metrics diff`
also per #933 (review)

* term: put Git ref exapmles before term and link
per #933 (review)

* cmd ref: friendlier explanation of "tip of default branch"
per #933 (review)

* cmd ref: use tag name instead of term "the revision"
per #933 (review)

* term: revert some "revision"->"reference" changes, and related simplifications
per #933 (review)

* cmd ref: review desc. of `-a` options throughout refs

* cmd ref: update diff params
per iterative/dvc/pull/3244

* cmd ref: update notes around moving/static Git refs in import and update
per #933 (review)

* revert workspace glossary entry
per #933 (review)

* tutorial: use full name of Deep Dive Tutorial in title and links
per #933 (review)

* user-guide: undo change on "binary" literal for analytics example
per #933 (review)

* use-cases: avoid term "revision" in data-registries
per #933 (review)

* term: avoid "checksum" in favor of file "hash" value
per #933 (review)

* term: SHA hash -> hash (Git commit context)
per #962 (comment)

Co-authored-by: Ruslan Kuprieiev <kupruser@gmail.com>
@jorgeorpinel
Copy link
Contributor Author

Please try to not dilute the focus of your PRs

Understood, sorry about that!

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.

3 participants