Skip to content

metrics: accept any viable target update#1797

Closed
pared wants to merge 2 commits into
treeverse:masterfrom
pared:4446_metrics
Closed

metrics: accept any viable target update#1797
pared wants to merge 2 commits into
treeverse:masterfrom
pared:4446_metrics

Conversation

@pared
Copy link
Copy Markdown
Contributor

@pared pared commented Sep 22, 2020

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

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

Related to treeverse/dvc#4590

@pared pared changed the title 4446 metrics metrics: accept any viable target update Sep 22, 2020
Comment on lines +59 to +61
Note, that when using `--targets` it is possible to `diff` files that are not
specifically marked as metrics.

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Sep 23, 2020

Choose a reason for hiding this comment

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

We can avoid this new paragraph by changing the beginning of the --targets option desc from

`--targets <paths>` - limit command scope to these metric files. Using -R
to something like
`--targets <paths>` - limit command scope to these metrics files (supports any file, even when not found as `metrics` in `dvc.yaml`). Using `-R`

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.

(Notice I also fixed the -R at the end, which should be in back quotes.)

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.

And problably we should include that info in dvc metrics show --help

Comment thread content/docs/command-reference/metrics/show.md Outdated
Copy link
Copy Markdown
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.

Thanks! Left some comments ☝️

If you're unable to further edit this soon, please reopen from a branch directly on this repo instead of a fork so it's easier to take it over.

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Sep 23, 2020

@pared Let's not forget to update plots docs as well, since the changes are so related.

@jorgeorpinel jorgeorpinel mentioned this pull request Sep 23, 2020
1 task
Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@pared
Copy link
Copy Markdown
Contributor Author

pared commented Sep 25, 2020

Closing in favor of #1809
Sorry @jorgeorpinel, keep forgetting to create PR's from origin.

@pared pared closed this Sep 25, 2020
@jorgeorpinel
Copy link
Copy Markdown
Contributor

Yeah no problem @pared wither way it works but if you open from origin it's easier for me to take it over if needed.

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