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

term: "metric" and related; other regular updates #611

Merged
merged 24 commits into from
Sep 18, 2019
Merged

Conversation

jorgeorpinel
Copy link
Contributor

No description provided.

@shcheklein shcheklein temporarily deployed to dvc-org-pr-611 September 9, 2019 06:27 Inactive
@jorgeorpinel jorgeorpinel changed the title term: "metric" and related term: "metric" and related; other regular updates Sep 9, 2019
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 9, 2019 06:34 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 9, 2019 08:00 Inactive
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

I've left a few comments. Especially please take a look at the comments in the get.md.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 9, 2019 18:49 Inactive
repo.

Note that the required `path` is expected to point to a file or directory
defined in one of the [DVC-files](/doc/user-guide/dvc-file-format) in the source
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shcheklein to your comment in 1fd6c11#r35008668,

defined -> controlled? referenced? I don't like that it sounds here like DVC-files come first and data second, while we always should be emphasizing that DVC-files are helpers/meta for data.

Hmmm OK updated here but we use this language in a bunch of other places, let me 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, reviewed throughout in my latest commit in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need to make a strict rule or fix other places. It's just in this case when we try to emphasize and explain that it expects data reference I would be careful with wording and usage of DVC-file stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm well it's not obvious to me when wording changes should or not propagate throughout all docs. (I like to do this generally, for consistency.) Do you think the default should be to avoid that? It's typically a quick "grep", but some times it can become pretty time consuming to check everything.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 9, 2019 18:54 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 9, 2019 20:02 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 11, 2019 17:19 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 11, 2019 17:20 Inactive
in the context of metric files (avoided).

per
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

a few minor questions

please, let me know when it's ready to be merged

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 13, 2019 22:49 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 16, 2019 21:51 Inactive
@shcheklein

This comment has been minimized.

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 17, 2019 06:20 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 17, 2019 06:43 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 17, 2019 06:50 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-611 September 17, 2019 06:52 Inactive
@jorgeorpinel

This comment has been minimized.

@shcheklein
Copy link
Member

@jorgeorpinel I think it's more or less ready to be merged - please resolve the conflicts and let me know when it's ready :)

@jorgeorpinel
Copy link
Contributor Author

Resolved (:

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

🎉 thanks!

@shcheklein shcheklein merged commit d56be6c into master Sep 18, 2019
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