cmd ref: get: document --show-url flag #936

skshetry · 2020-01-20T07:12:45Z

Fixes #930

Disregard the recommendations below if you use Edit on GitHub button to improve the docs 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 enables GitHub to link the PR to the corresponding bug and close it automatically when PR is merged.

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

public/static/docs/command-reference/get.md

shcheklein

@skshetry thanks!! 🙏 if you have a few minutes feel free to address some comments, otherwise consider these comments for me and @jorgeorpinel :)

jorgeorpinel

Thanks for the initial version @skshetry! Looking good in general but there are some language improvements that could be made 🙂 I also left extra comments in some of Ivan's reviews above.

public/static/docs/command-reference/get.md

shcheklein · 2020-01-21T18:17:08Z

@skshetry @jorgeorpinel Saugat, are you okay if we start iterating on this with you? Jorge will put some modifications and will ask you to review before merging.

skshetry · 2020-01-21T18:21:01Z

@shcheklein, yes, if that'd be faster. I was just waiting for replies on the discussions before working on this.

shcheklein · 2020-01-21T18:25:16Z

@skshetry @jorgeorpinel yes, I think it would be faster - your first iteration -> our changes applied -> you review and suggest fixes -> merge :) otherwise we spend too much time iterating for no reason. The most important here is that you review the last iteration to learn and teach us :)

skshetry · 2020-01-21T18:26:36Z

@shcheklein, okay, makes sense. 🙂

jorgeorpinel · 2020-01-22T18:40:59Z

I think it would be faster - your first iteration -> our changes applied -> you review and suggest fixes -> merge

It is faster for small changes but not necessarily when we need to rethink the phrasing, etc. (because the docs team is small and can easily get backed up). But yep, I'll take over this one ASAP.

The CI format-check is failing though (and Restyled didn't catch it, weird). Please take a look: https://circleci.com/gh/iterative/dvc.org/1890

Note: please use a branch directly on this repo instead of your fork in future PRs @skshetry – Ivan can give you access if needed. It makes it easier for others to contribute to the PR. Thanks

shcheklein · 2020-01-22T18:50:43Z

@jorgeorpinel

when we need to rethink the phrasing

in certain cases we can merge it then, even if it's not 100% perfect. It would be taking too long to expect from everyone to write the text of the highest quality.

per iterative#936 (review) and iterative#936 (review) also applied iterative#936 (review) and iterative#936 (comment)

jorgeorpinel · 2020-01-23T03:00:16Z

I wasn't referring to language quality but to clarify the idea and include all the information needed, even if it will be rewritten with different wording. Some reviews include this kind of questions which is easier/faster for the person who developed the change to include.

per iterative#936 (comment)

jorgeorpinel · 2020-01-23T03:07:34Z

I'll take over this one ASAP.

Ok, I have now, and addressed all the feedback. Please review @shcheklein @skshetry. Thanks

skshetry

Thanks @jorgeorpinel. Looks good.

shcheklein · 2020-01-23T04:26:01Z

public/static/docs/command-reference/get.md

+- `--show-url` - `url` is expected to represent a DVC project for this option to
+  have an effect (as opposed to a Git-only, non-DVC repository). When
+  `--show-url` is used, instead of downloading the file or directory, this
+  command just prints the storage location (URL) of the target data.


I still think we are overcomplicating this. The fact that it does not handle Git repos is only somewhat important and at least should go after the essence of what this command does. Also, probably it's not about DVC repo vs Git repo - it's about Git-tracked files vs DVC-tracked files. Am I correct here, @skshetry ?

Also, consider including a link the example below.

No, it's about DVC repo vs Git repo. As an example, take this git repo. How can you get an url for this public/static/docs/command-reference/get.md? Git does not work with files, so there's no URL/path to generate for (unless we do somekind of magic).

okay, to clarify it's not about Git-only vs DVC. Any Git+DVC has files that are not under DVC control and probably we don't generate URL for them, right?

I'm not even sure about DVC-controlled non-cached files (dvc run -O something), probably we don't even handle that with this new option (even though we can download it)

Yes, that should not also work, as it checks for all DVC controlled outputs.

So, it's also about Git vs DVC tracked files. Sorry about that.

The fact that it does not handle Git repos is only somewhat important and at least should go after the essence of what this command does.

@shcheklein You may be right but about this we're already using this style in most other options when there are pre-requisites. Putting the pre-req's as the first sentence. (See --rev just above this.) That's not a great reason to keep this but I'm also not convinced that leaving the pre-req's for last is best. Maybe it is better to leave them on top? they are "pre" requisites after all 🙂

All that said I did shorten that first sentence now, and also the main explanation following it.

to clarify it's not about Git-only vs DVC. Any Git+DVC has files that are not under DVC control

True, I updated that as well. So the pre-req is not about url then, but about path: it should be a DVC-tracked file/dir.

See ab40ed7.

I'm not even sure about DVC-controlled non-cached files (dvc run -O something), probably we don't even handle that with this new option (even though we can download it)

Actually, both this option and get_url() should support creating a URL for these. They don't atually check for the existence of the file in remote cache. They just interpret the project configured remote + the checksum in DVC-file from what I understand. Cc @shcheklein

shcheklein

@skshetry @jorgeorpinel please, check the last comment

per iterative#936 (review)

public/static/docs/command-reference/get.md

efiop · 2020-01-24T10:31:10Z

@shcheklein @jorgeorpinel Considering that Saugat is away for ~2weeks, could we merge it and work on top later?

per iterative#936 (review)

shcheklein · 2020-01-24T19:06:37Z

@efiop yep, it's on us now, no expectations from the eng team at this point.

per previous 2 commits

jorgeorpinel · 2020-01-24T19:16:29Z

Docs team for the win 🎉

public/static/docs/command-reference/pull.md

public/static/docs/command-reference/push.md

cmd ref: get: document --show-url flag

78055d4

skshetry requested a review from shcheklein January 20, 2020 07:12

skshetry self-assigned this Jan 20, 2020

skshetry requested a review from jorgeorpinel January 20, 2020 07:13

skshetry commented Jan 20, 2020

View reviewed changes

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

shcheklein reviewed Jan 20, 2020

View reviewed changes

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

shcheklein reviewed Jan 20, 2020

View reviewed changes

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

shcheklein requested changes Jan 20, 2020

View reviewed changes

jorgeorpinel mentioned this pull request Jan 20, 2020

get: implement --show-url to display only url/path to remote iterative/dvc#3156

Merged

4 tasks

jorgeorpinel suggested changes Jan 20, 2020

View reviewed changes

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

cmd ref: simplify and improve --show-url description, et al.

072defa

per iterative#936 (review) and iterative#936 (review) also applied iterative#936 (review) and iterative#936 (comment)

cmd ref: update get --show-url example

663f589

per iterative#936 (comment)

jorgeorpinel requested a review from shcheklein January 23, 2020 03:07

jorgeorpinel approved these changes Jan 23, 2020

View reviewed changes

skshetry commented Jan 23, 2020

View reviewed changes

shcheklein reviewed Jan 23, 2020

View reviewed changes

shcheklein requested changes Jan 23, 2020

View reviewed changes

jorgeorpinel added 2 commits January 23, 2020 11:30

cmd ref: clarify pre-requisite of --show-url option

ab40ed7

per iterative#936 (review)

formatting?

9ba3f5d

shcheklein reviewed Jan 23, 2020

View reviewed changes

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

shcheklein reviewed Jan 23, 2020

View reviewed changes

public/static/docs/command-reference/get.md Outdated Show resolved Hide resolved

jorgeorpinel added a commit to maxhora/dvc.org that referenced this pull request Jan 24, 2020

cmd ref: remove prereq sentence from get --rev option

1a76ce2

per iterative#936 (review)

jorgeorpinel added 2 commits January 24, 2020 13:05

cmd ref: remove prereq sentence from get --rev option

25b65cb

per iterative#936 (review)

move prereq sentence to last in get --show-url option

cab347c

per iterative#936 (review)

shcheklein approved these changes Jan 24, 2020

View reviewed changes

cmd ref: standart use or prereqs in all cmd options

2615671

per previous 2 commits

jorgeorpinel merged commit b0575dd into iterative:master Jan 24, 2020

weekly-digest bot mentioned this pull request Jan 26, 2020

Weekly Digest (19 January, 2020 - 26 January, 2020) #949

Closed

skshetry commented Feb 7, 2020

View reviewed changes

public/static/docs/command-reference/pull.md Show resolved Hide resolved

skshetry commented Feb 7, 2020

View reviewed changes

public/static/docs/command-reference/push.md Show resolved Hide resolved

skshetry deleted the doc-get-show-url-ref branch February 7, 2020 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd ref: get: document --show-url flag #936

cmd ref: get: document --show-url flag #936

skshetry commented Jan 20, 2020

This comment has been minimized.

This comment has been minimized.

shcheklein left a comment

jorgeorpinel left a comment •

edited

Loading

shcheklein commented Jan 21, 2020

skshetry commented Jan 21, 2020

shcheklein commented Jan 21, 2020

skshetry commented Jan 21, 2020

jorgeorpinel commented Jan 22, 2020

shcheklein commented Jan 22, 2020

jorgeorpinel commented Jan 23, 2020 •

edited

Loading

jorgeorpinel commented Jan 23, 2020

skshetry left a comment

shcheklein Jan 23, 2020

skshetry Jan 23, 2020

shcheklein Jan 23, 2020

shcheklein Jan 23, 2020

skshetry Jan 23, 2020

skshetry Jan 23, 2020

jorgeorpinel Jan 23, 2020

This comment was marked as resolved.

jorgeorpinel Jan 23, 2020 •

edited

Loading

shcheklein left a comment

efiop commented Jan 24, 2020

shcheklein commented Jan 24, 2020

jorgeorpinel commented Jan 24, 2020

cmd ref: get: document --show-url flag #936

cmd ref: get: document --show-url flag #936

Conversation

skshetry commented Jan 20, 2020

This comment has been minimized.

This comment has been minimized.

shcheklein left a comment

Choose a reason for hiding this comment

jorgeorpinel left a comment • edited Loading

Choose a reason for hiding this comment

shcheklein commented Jan 21, 2020

skshetry commented Jan 21, 2020

shcheklein commented Jan 21, 2020

skshetry commented Jan 21, 2020

jorgeorpinel commented Jan 22, 2020

shcheklein commented Jan 22, 2020

jorgeorpinel commented Jan 23, 2020 • edited Loading

jorgeorpinel commented Jan 23, 2020

skshetry left a comment

Choose a reason for hiding this comment

shcheklein Jan 23, 2020

Choose a reason for hiding this comment

skshetry Jan 23, 2020

Choose a reason for hiding this comment

shcheklein Jan 23, 2020

Choose a reason for hiding this comment

shcheklein Jan 23, 2020

Choose a reason for hiding this comment

skshetry Jan 23, 2020

Choose a reason for hiding this comment

skshetry Jan 23, 2020

Choose a reason for hiding this comment

jorgeorpinel Jan 23, 2020

Choose a reason for hiding this comment

This comment was marked as resolved.

jorgeorpinel Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

shcheklein left a comment

Choose a reason for hiding this comment

efiop commented Jan 24, 2020

shcheklein commented Jan 24, 2020

jorgeorpinel commented Jan 24, 2020

jorgeorpinel left a comment •

edited

Loading

jorgeorpinel commented Jan 23, 2020 •

edited

Loading

jorgeorpinel Jan 23, 2020 •

edited

Loading