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

get/import: updates output strings, et al. #3308

Merged
merged 13 commits into from
Feb 17, 2020
Merged

get/import: updates output strings, et al. #3308

merged 13 commits into from
Feb 17, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Feb 11, 2020 •
edited
Loading

Continuation of #3277

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

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

@jorgeorpinel jorgeorpinel changed the title Regular updates (output strings et al.) get/import: updates output strings, et al. Feb 11, 2020
@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Feb 11, 2020 •
edited
Loading

This just matches existing published docs (newer than the core code) as well as the upcoming metrics diff docs PR (iterative/dvc.org/pull/933).

@pared
Copy link
Contributor

pared commented Feb 12, 2020

No that is part of this change, but maybe we should change version-> revision in diff's descriptions? From discussion under (#3245) it seems that revision is the general term used in code. Also, both in dvc diff and dvc metrics diff positional arguments suggest rev. Also, that seems to be the term in docs. version on the other hand is mentioned only in dvc version.

@jorgeorpinel
Copy link
Contributor Author

maybe we should change version-> revision in diff's descriptions... that seems to be the term in docs

We prefer version when possible, its more user friendly. See #3245 (comment). It's still in docs bc the iterative/dvc.org/pull/933 PR is not merged yet.

both in dvc diff and dvc metrics diff positional arguments suggest rev

Thanks, I updated the autocomplete scripts now.

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Feb 13, 2020 •
edited
Loading

NOTE: Please don't merge this yet, just in case there are more related changes in iterative/dvc.org/pull/933. Will ping you when ready, Pawel and Ruslan.

@efiop efiop mentioned this pull request Feb 13, 2020
Merged
@jorgeorpinel jorgeorpinel requested review from pared and efiop February 14, 2020 05:44
@jorgeorpinel
Copy link
Contributor Author

Ready for final review @pared @efiop! Thanks

@jorgeorpinel jorgeorpinel mentioned this pull request Feb 14, 2020
Merged
3 tasks
@weekly-digest weekly-digest bot mentioned this pull request Feb 16, 2020
Closed
efiop pushed a commit that referenced this pull request Feb 17, 2020
@jorgeorpinel
term: avoid "under X control" and other help output updates (#3322)
b23f43a 
* get/import: update cmd and url arg descs with correct behavior

* get/import: update path arg desc as well (See previous commit)

* get/imp: udpate help output with simpler terms
per #3277 (review)

* api: url->URL in a docstring

* metrics show: update help output per
per iterative/dvc.org#933 (review)

* diff: update `diff` and `metrics diff` help desc to match docs
per iterative/dvc.org#933 (review)

* diff: update autocomplete scripts
per #3308 (comment)

* diff: update help output again
per iterative/dvc.org#933 (review)

* diff/metrics diff: update help output yet again
per iterative/dvc.org#933 (comment)

* term" under X control -> tracked by X
@efiop efiop merged commit a663971 into iterative:master Feb 17, 2020
@weekly-digest weekly-digest bot mentioned this pull request Feb 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pared pared pared approved these changes

@shcheklein shcheklein shcheklein approved these changes

@efiop efiop Awaiting requested review from efiop

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jorgeorpinel @pared @shcheklein @efiop