Skip to content

dvc: update cmd output strings to match docs#3471

Merged
efiop merged 14 commits into
treeverse:masterfrom
jorgeorpinel:2020-02-21
Mar 23, 2020
Merged

dvc: update cmd output strings to match docs#3471
efiop merged 14 commits into
treeverse:masterfrom
jorgeorpinel:2020-02-21

Conversation

@jorgeorpinel
Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel commented Mar 11, 2020

to match treeverse/dvc.org/pull/1014
and now treeverse/dvc.org/pull/1040 too

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

Comment thread dvc/command/metrics.py Outdated
metrics_add_parser.set_defaults(func=CmdMetricsAdd)

METRICS_MODIFY_HELP = "Modify metric file options."
METRICS_MODIFY_HELP = "Modify metric file values."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it should be options.

modifying values sounds like we actually modify metric values.

it is about setting up metric file settings (type, path, etc), not modifying metric values

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see. I'll update this... ⏳ but still need the PR for other strings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about "default formatting"? See latest version here and in treeverse/dvc.org/pull/1014

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

default formatting options is still better? )

Copy link
Copy Markdown
Contributor Author

@jorgeorpinel jorgeorpinel Mar 11, 2020

Choose a reason for hiding this comment

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

I'm trying to avoid term "option" here since there are also command options (CLI flags) and config options (DVC settings) and it gets confusing some times in the explanations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, I got the idea. I think it's almost impossible to avoid unfortunately in general (even if it works in this case). At least I was not able to find a good solution - may be even primarily I never thought that it's a really big deal - usually it's clear from the context. And can add more words when it's not clear from the context I think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I managed to avoid the term everywhere actually 🙂 assuming we let this one through. Yes it's not always a big deal but in this case and others I changed I thought it was confusing since all the "option" concepts are used in the same doc or even section of the doc sometimes.

using term "formatting" instead of "options"
jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Mar 11, 2020
@jorgeorpinel jorgeorpinel changed the title metrics: update output strings to match iterative/dvc.org/pull/1014 metrics: update output strings to match docs Mar 12, 2020
Comment thread dvc/command/ls.py Outdated
Comment thread dvc/external_repo.py Outdated
@jorgeorpinel jorgeorpinel requested a review from efiop March 12, 2020 00:33
@jorgeorpinel jorgeorpinel changed the title metrics: update output strings to match docs commands: update output strings to match docs Mar 12, 2020
Comment thread dvc/command/data_sync.py

# Pull
PULL_HELP = "Pull data files from a DVC remote storage."
PULL_HELP = "Download tracked files or directories from remote storage."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure pull and fetch help messages are different enough now. But it is definitely not worse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm just copying from https://dvc.org/doc/command-reference/fetch and https://dvc.org/doc/command-reference/pull here. This whole PR is about matching docs which I think is desirable but if not, please lmk!

Comment thread dvc/command/ls/__init__.py Outdated
list_parser.add_argument(
"url",
help="Supported urls:\n"
"/path/to/file\n"
Copy link
Copy Markdown
Contributor

@efiop efiop Mar 18, 2020

Choose a reason for hiding this comment

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

Not sure why are we getting rid of these useful examples. In favor of docs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah for consistency with get and import and so the output here is shorter. We could go the opposite way and list all the examples in all these commands if that's preferable. Or we could add a link to the cmd ref in each one. Please lmk, happy to change it.

Comment thread dvc/command/metrics.py
Comment thread dvc/command/metrics.py Outdated
metrics_modify_parser.set_defaults(func=CmdMetricsModify)

METRICS_REMOVE_HELP = "Remove files's metric tag."
METRICS_REMOVE_HELP = "Stop tracking a metric file."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might be confusing. We are not stopping tracking the file but rather un-marking it as a metric.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True. I'm updating this one and metrics add, as well as the cmd refs. See f262f48 and treeverse/dvc.org/pull/1072. Thanks!

Comment thread dvc/command/pipeline.py
pipeline_show_parser.set_defaults(func=CmdPipelineShow)

PIPELINE_LIST_HELP = "List pipelines."
PIPELINE_LIST_HELP = "List connected groups of stages (pipelines)."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are we making this clarification here, but not in other places? pipeline is self descriptive enough, I don't think it needs a "connected groups of stages"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question. Again, this is to match docs, https://dvc.org/doc/command-reference/pipeline/list specifically.

but not in other places

The only other place really is pipeline show which reads "Show stages in a pipeline." This one also mentions both terms "stage" and "pipeline" so it's similarly clear, I think.

Comment thread dvc/command/remote.py
fix_subparsers(remote_subparsers)

REMOTE_ADD_HELP = "Add remote."
REMOTE_ADD_HELP = "Add a new data remote."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we sure we need data here? Won't it be confusing in regards to models and such?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry to repeat the same answer haha but just matching docs here (https://dvc.org/doc/command-reference/remote/add).

For consistency, we use the term "data remote" in remote subcommands cmd refs (not in other docs though). This is kind of arbitrary, I agree on that. Would you prefer a different standard? Happy to update everywhere.

Comment thread dvc/command/remove.py Outdated

def add_parser(subparsers, parent_parser):
REMOVE_HELP = "Remove DVC-file outputs."
REMOVE_HELP = "Remove data files or directories tracked by DVC."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not only about data, but about stages/models/etc too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmmm OK changed to "Remove DVC-tracked files or directories." both here and in the cmd ref now. Better? The part about removing entire stage files is explained in the cmd ref desc.

Comment thread dvc/command/repro.py
def add_parser(subparsers, parent_parser):
REPRO_HELP = "Check for changes and reproduce stages and dependencies."
REPRO_HELP = "Reproduce complete or partial pipelines"
" by executing their stages."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe running?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we have dvc run command, we use the term "execute" a lot in the repro cmd ref (https://dvc.org/doc/command-reference/repro).

Comment thread dvc/command/root.py Outdated
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Mar 18, 2020

Sorry for the delay.

@jorgeorpinel jorgeorpinel changed the title commands: update output strings to match docs dvc: update cmd output strings to match docs Mar 19, 2020
jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Mar 19, 2020
jorgeorpinel added a commit to treeverse/dvc.org that referenced this pull request Mar 19, 2020
@jorgeorpinel jorgeorpinel requested a review from efiop March 19, 2020 06:54
@efiop efiop merged commit 9e77405 into treeverse:master Mar 23, 2020
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Mar 23, 2020

Ok, merging for now. Will try to keep an eye on the docs PRs.

@jorgeorpinel jorgeorpinel deleted the 2020-02-21 branch October 12, 2020 00:18
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.

4 participants