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

get: add example on downloading normal git files #821

Merged
merged 2 commits into from
Dec 9, 2019

Conversation

danihodovic
Copy link
Contributor

@danihodovic danihodovic commented Nov 26, 2019

@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
static/docs/command-reference/get.md Outdated Show resolved Hide resolved
static/docs/command-reference/get.md Outdated Show resolved Hide 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.

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
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
Member

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

@jorgeorpinel
Copy link
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 iterative/dvc#2837 (comment) @shcheklein.

@danihodovic
Copy link
Contributor Author

@jorgeorpinel what changes exactly in the dvc repo?

@jorgeorpinel
Copy link
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
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 iterative/dvc#2837

@jorgeorpinel
Copy link
Contributor

iterative/dvc/pull/2837 🙂

Copy link
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.

static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
static/docs/command-reference/get.md Show resolved Hide resolved
@jorgeorpinel
Copy link
Contributor

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

@danihodovic
Copy link
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.

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

@jorgeorpinel
Copy link
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 iterative/dvc#2837 in any case.

@shcheklein
Copy link
Member

@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
Contributor

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

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

@jorgeorpinel jorgeorpinel merged commit 8497023 into iterative:master Dec 9, 2019
jorgeorpinel added a commit that referenced this pull request Dec 10, 2019
@jorgeorpinel
Copy link
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