Skip to content

get: add example on downloading normal git files#821

Merged
jorgeorpinel merged 2 commits into
treeverse:masterfrom
danihodovic:docs/dvc-get-normal-file
Dec 9, 2019
Merged

get: add example on downloading normal git files#821
jorgeorpinel merged 2 commits into
treeverse:masterfrom
danihodovic:docs/dvc-get-normal-file

Conversation

@danihodovic
Copy link
Copy Markdown
Contributor

@danihodovic danihodovic commented Nov 26, 2019

Comment thread static/docs/command-reference/get.md Outdated
Comment thread static/docs/command-reference/get.md Outdated
Comment thread static/docs/command-reference/get.md Outdated
@jorgeorpinel jorgeorpinel changed the title cmd ref: add examples on downloading normal git files get: add example on downloading normal git files Nov 26, 2019
Comment thread static/docs/command-reference/get.md Outdated
Comment thread static/docs/command-reference/get.md Outdated
Copy link
Copy Markdown
Contributor

@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'm fine with obtain. My 2cs though - I would not complicate it and use Download term. May be mention in the description that in certain cases it can be optimized and avoid actual network IO.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

I already convinced everyone to use "download/copy" 😅 but yeah I also came to that conclusion recently... I'll just open a separate issue to deal with "download/copy"...

@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel @danihodovic if it's a small change I think it can be done as part of this ticket?

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Nov 28, 2019

It involves changes in the dvc repo as well as dvc import probably though, so not necessarily that small. But we can try, yes.

p.s. See treeverse/dvc#2837 (comment) @shcheklein.

@danihodovic
Copy link
Copy Markdown
Contributor Author

@jorgeorpinel what changes exactly in the dvc repo?

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Nov 28, 2019

Command output strings.

Also I'm still not sure what's best. "Download", "obtain", or "download/copy". I kind of like obtain. My intention is to extract this discussion to another issue (that may also apply to the other 3 related commands: get, get-url, and import-url).

@danihodovic
Copy link
Copy Markdown
Contributor Author

danihodovic commented Nov 28, 2019

My intention is to extract this discussion to another issue (that may also apply to the other 3 related commands: get, get-url, and import-url).

I agree with this. I don't want a long discussion on terminology to block the code changes in treeverse/dvc#2837

@jorgeorpinel
Copy link
Copy Markdown
Contributor

treeverse/dvc/pull/2837 🙂

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.

Some more small things.

Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
Comment thread static/docs/command-reference/get.md
@jorgeorpinel
Copy link
Copy Markdown
Contributor

I've extracted the discussion about terminology to #825.

@danihodovic
Copy link
Copy Markdown
Contributor Author

danihodovic commented Nov 28, 2019

I've extracted the discussion about terminology to #825.

Thanks. I'd prefer to put this on hold until the discussion on terminology has been settled. I feel like I'm changing things back and forth for no good reason.

treeverse/dvc#2837 can be merged without this PR as it shouldn't break backwards compatibility or modify existing functionality.

@jorgeorpinel
Copy link
Copy Markdown
Contributor

jorgeorpinel commented Nov 28, 2019

No need to put this on hold, please address the review because who knows when we'll get to that other issue. Sorry for the confusion but the back and forth is due to a misunderstanding: I never meant to just replace all the instances of "download" for "obtain" without considering their context. But if you prefer to just revert to "download" everywhere for now, that's also OK. Thanks Dani.

p.s. The usage should match the changes on treeverse/dvc#2837 in any case.

@shcheklein
Copy link
Copy Markdown
Contributor

@jorgeorpinel could you please summarize what else should be addressed here? Should we merge and fix what's left as part of your regular update?

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Should we merge and fix what's left as part of your regular update

Yes, let's do this. Same strategy as in treeverse/dvc/pull/2837 🙂

@jorgeorpinel jorgeorpinel merged commit 8497023 into treeverse:master Dec 9, 2019
jorgeorpinel added a commit that referenced this pull request Dec 10, 2019
@jorgeorpinel
Copy link
Copy Markdown
Contributor

Addressed my pending review in a9a9bda.

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