Skip to content

api: add list command#967

Closed
gurobokum wants to merge 1 commit into
treeverse:masterfrom
gurobokum:dvc_list
Closed

api: add list command#967
gurobokum wants to merge 1 commit into
treeverse:masterfrom
gurobokum:dvc_list

Conversation

@gurobokum
Copy link
Copy Markdown
Contributor

Relates:


❗ 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. 🙏

Comment thread public/static/docs/command-reference/list.md Outdated
@@ -0,0 +1,40 @@
# list

List file structure of a repo including <abbr>data artifacts</abbr>

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

dvc list can list simple folder (without .git), shouldn't it be pointed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would mention it in the description - that's what it is for - to elaborate and expand

Comment thread public/static/docs/command-reference/list.md Outdated
Comment thread public/static/docs/command-reference/list.md Outdated

## Description

The command list files and <abbr>data artifacts</abbr> into pointed repo.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

need to expand and elaborate here. No need to repeat just the same phrase we used to above. Even http://man7.org/linux/man-pages/man1/ls.1.html has something meaningful and non-repetitive here.

Would be great to expand on the fact that we show actual data behind DVC-files. That it can be used before dvc get to see that file name to path (which is not easy with Github), etc.

Comment thread public/static/docs/command-reference/list.md Outdated

- `--outputs-only` - Show only <abbr>data artifacts</abbr>.

- `-R`, `--recursive` - Recursively traverse the repo.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the repo -> the target? actually it's is the path (that's why I don't like the target name for the CLI argument)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

check the aws s3 ls and ls man pages. traverse is not the right verb here ... print, list, etc are better

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree. If target can only be a directory path, why not call it path or dir? (Also commenting in /pull/967)

Comment thread public/static/docs/command-reference/list.md Outdated
Comment thread public/static/docs/command-reference/list.md
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.

@JIoJIaJIu good stuff, see some comments please!

@jorgeorpinel
Copy link
Copy Markdown
Contributor

Please let us know when treeverse/dvc/pull/3246 is merged to check this doc vs the final behavior of the command 🙂

@gurobokum
Copy link
Copy Markdown
Contributor Author

@shcheklein @jorgeorpinel Could you take a look please on the PR

@gurobokum gurobokum changed the title [WIP] api: add list command api: add list command Feb 20, 2020
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.

looks good to me! I think we can take if from here @jorgeorpinel ?

Comment thread public/static/docs/command-reference/list.md
Comment thread public/static/docs/command-reference/list.md

- `--outputs-only` - Show only <abbr>data artifacts</abbr>.

- `-R`, `--recursive` - Recursively traverse the repo.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree. If target can only be a directory path, why not call it path or dir? (Also commenting in /pull/967)

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.

Sure @shcheklein, let's.

Some first basic comments on this for now (above) ^

@jorgeorpinel
Copy link
Copy Markdown
Contributor

@JIoJIaJIu any chance you could move this to a regular branch in this repo though? If not, it's OK.

Please check out https://github.com/iterative/dvc.org/wiki/2.-dvc.org-pull-requests-for-core-repo-changes for future ref. 🙂

@gurobokum
Copy link
Copy Markdown
Contributor Author

@jorgeorpinel @shcheklein

any chance you could move this to a regular branch in this repo though

I've create a new PR - #1021 and close this one

Agree. If target can only be a directory path, why not call it path or dir?

I suggest to apply my current PR and in the next iteration I will rename target to path, at this moment I am blocked with the treeverse/dvc#3351 for being able to refactor the source code. As soon it is merged I will change all target entries to path and update the docs

@gurobokum gurobokum closed this Feb 25, 2020
@jorgeorpinel
Copy link
Copy Markdown
Contributor

Thanks Guro!

I suggest to apply my current PR and in the next iteration I will rename target to path

Yep no rush, it's included in treeverse/dvc/issues/3381 actually so it should be addressed whenever someone takes that issue.

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.

4 participants