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.

Sign up for GitHub

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

Jump to bottom

diff: use rev instead of ref #3299

Merged
merged 1 commit into from
Feb 10, 2020
Merged

diff: use rev instead of ref #3299

merged 1 commit into from
Feb 10, 2020

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Feb 10, 2020 •
edited by jorgeorpinel
Loading

Fixes #3245

  • ❗ 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.
    Done in efiop/dvc.org@d52dc5a

  • ❌ 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.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@efiop efiop requested a review from jorgeorpinel February 10, 2020 18:14
jorgeorpinel
jorgeorpinel approved these changes Feb 10, 2020
View reviewed changes
Copy link
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.

Fixes #3245

I think this almost gets there, but to 100% close that issue should we even rename rev arguments to commit pr cmt perhaps? I'm not sure. Cc @shcheklein

dvc/repo/diff.py Show resolved Hide resolved
@shcheklein
Copy link
Member

@johntharian I think we discussed this already and agreed on --rev? Otherwise it will be way bigger change (will involve APIs, get/import, update, etc, etc). I don't see a strong reason do this.

@jorgeorpinel
Copy link
Contributor

OK.

Unrelated: wrong handle there though haha. Why does GH suggest users randomly instead of the ones we work with the most?

@efiop efiop merged commit 0c88979 into iterative:master Feb 10, 2020
@efiop efiop deleted the ref_rev branch February 10, 2020 22:13
@efiop
Copy link
Contributor Author

efiop commented Feb 10, 2020

@jorgeorpinel Btw, is there a WIP PR that fixes the naming in the docs? If not, I could send a PR to fix stuff related to this PR.

@jorgeorpinel jorgeorpinel mentioned this pull request Feb 10, 2020
Closed
@jorgeorpinel

This comment has been minimized.

Sign in to view
@shcheklein

This comment has been minimized.

Sign in to view
@shcheklein
Copy link
Member

I see we also changed some help messages. Yes, then we should change them all indeed.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 11, 2020 •
edited
Loading

is there a WIP PR that fixes the naming in the docs?

Nope, good catch @efiop, this needs a corresponding PR in docs indeed (unchecked the 2nd checkbox). Thanks!

the intention was to change ref to rev

Right, that's what the title suggests @shcheklein but it was also marked as fixing term: replace (Git) "reference" and "revision" for "commit" #3245 so I wasn't sure 🙂

Anyway, I reopenned that issue and indicated the missing instancs – if we're doing that now maybe the corresponding docs update can also include those BTW, Ruslan. Again, lmk if you need help, it looks like a small thing I could do myself (both core and docs).

@efiop efiop self-assigned this Feb 11, 2020
@efiop efiop added refactoring Factoring and re-factoring p0-critical Critical issue. Needs to be fixed ASAP. labels Feb 11, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Feb 11, 2020
Merged
@jorgeorpinel
Copy link
Contributor

is there a WIP PR that fixes the naming in the docs? If not, I could send a PR to fix stuff related to this PR.

this needs a corresponding PR in docs indeed (unchecked the 2nd checkbox)

Hey @efiop don't worry about it, I'll do this myself in iterative/dvc.org/pull/933

jorgeorpinel added a commit to efiop/dvc.org that referenced this pull request Feb 12, 2020
@jorgeorpinel
cmd ref: change "ref" -> "rev" per iterative/dvc/pull/3299
d52dc5a 
and iterative#933 (review)
shcheklein pushed a commit to iterative/dvc.org that referenced this pull request Feb 13, 2020
@efiop @jorgeorpinel
add docs for dvc metrics diff (#933)
bd0e250 
* 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 <[email protected]>
@efiop
Copy link
Contributor Author

efiop commented Feb 13, 2020

@jorgeorpinel Sorry, was about to submit a PR for that. Will clarify this before merging next time. Thank you!

This was referenced Feb 16, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgeorpinel jorgeorpinel jorgeorpinel approved these changes

Assignees

@efiop efiop

Labels
p0-critical Critical issue. Needs to be fixed ASAP. refactoring Factoring and re-factoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

term: replace (Git) "reference" for "revision" or "commit"
3 participants
@efiop @shcheklein @jorgeorpinel