Skip to content

Conversation

@fabiosantoscode
Copy link
Contributor

@fabiosantoscode fabiosantoscode commented Jan 10, 2020

Closes #911, and documents non-merged dvc get feature implemented in treeverse/dvc#3097

@jorgeorpinel

This comment has been minimized.

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.

Please reflect the changes from https://github.com/iterative/dvc/pull/3097/files here 🙂

@fabiosantoscode
Copy link
Contributor Author

Hi @jorgeorpinel, I've reflected the usage, and looked for other instances of the output using git grep 'dvc get' and git grep 'dvc import'.

Everything should be up to date.

I've referred to "DVC or Git" repositories as "Git repositories or DVC repositories" if the sentence is introductory, and simply as "Git repositories" if the sentence is advanced. Makes sense to me to be less terse when introducing a topic than when giving details about it.

@fabiosantoscode
Copy link
Contributor Author

Ok, so now when describing a Git or DVC repository, it's "Git/DVC repository". I've also found a place where I was including Git for no good reason (in the "data registries" use case, where a git repository without DVC should not be mentioned).

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.

Commented on some of Ivan's reviews above, added my own below, and here are a couple more notes (several having to do with free find/replace text – context is very important in docs 🙂):

  • The paragraph explaining url, starting in line 31 below, also needs updating. Currently:

    The url argument specifies the address of the Git repository containing the external project...

  • The one about path in line 39+ also needs updating to something like:

    The path argument of this command is used to specify the location, within the source project or repository at url...

Both checkboxes apply to both dvc get and dvc import.

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.

Thanks for trying to change throughout all docs @fabiosantoscode it was definitely the right thing to do, but there's still some unnecessary changes given the context. I've left another full review...

Ok, so now when describing a Git or DVC repository, it's "Git/DVC repository".

It has the right intention but it has some problems; It's a bit simplistic; We want to be able to use <abbr> around "DVC repository"; We want to emphasize DVC first; It's confusing: DVC repo is already a Git repo. Anyway, let's just use a full phrase like I mentioned in #912 (comment).

WARNING – RANT: ^ This is not your fault Fabian. I think the change of behavior has made dvc get and dvc import kind of confusing in a new way. Why are we building a Git download wrapper? I know there were reasons but in retrospect this seems strange. Anyway, I will just have to review both command references again in a future PR. I'll open a separate issue for this...

Also:

  • The one about path in line 39+ also needs updating to something like...
    (See my previous #912 (review))

Still pending ^ (along with a few of the comments inside that review).

Getting there for sure.

fabiosantoscode and others added 3 commits January 14, 2020 20:56
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@fabiosantoscode
Copy link
Contributor Author

fabiosantoscode commented Jan 14, 2020

Thanks for all the suggestions!

I've replied below each change request, and to reply to the review comment itself: I've now fixed those two situations.

I didn't use find and replace without reading around the sites, but should probably have spent more time reading. Also, just because you can dvc import a data file in a remote plain-git repository, doesn't mean you should :) I should've taken that into consideration.

edit: I didn't mark the conversations as resolved because that would obscure what I've replied to. This page is getting pretty confusing :)

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.

Thanks again @fabiosantoscode, this is pretty much perfect now!

Comment on lines +24 to +29
Provides an easy way to reuse files or directories tracked in any <abbr>DVC
project</abbr> (e.g. datasets, intermediate results, ML models) or Git
repository (e.g. other files and directories), into the workspace. The
`dvc import` command downloads such a <abbr>data artifact</abbr> in a way that
it is tracked with DVC, so it can be updated when the data source changes. (See
`dvc update`.)
Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 15, 2020

Choose a reason for hiding this comment

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

OK this is almost perfect actually, I'll just make some minor updates in a future PR.

p.s. from #912 (comment) :

If we need to write a second paragraph for the Git repo case that's one option, or making this one clear enough if possible... I would try to keep it as similar as possible (almost copy paste) of the latest intro paragraph in dvc get.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done in 44e7fca.

> find the appropriate [remote storage](/doc/command-reference/remote) and
> download <abbr>data artifacts</abbr> from it. (It works like `wget`, but for
> DVC repositories.) In this case we use
> DVC or Git repositories.) In this case we use
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll change this back to DVC repo and put <abbr> around it so the tooltip (which mentions DVC repos are Git repos) comes up.

Copy link
Contributor

Choose a reason for hiding this comment

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

See 96232e0

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.

get: it can download from plain (non-DVC) Git repos.

3 participants