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

api: add list command #967

Closed
wants to merge 1 commit into from
Closed

api: add list command #967

wants to merge 1 commit into from

Conversation

gurobokum
Copy link
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. πŸ™

@@ -0,0 +1,40 @@
# list

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

This comment was marked as resolved.

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

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


## Description

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

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.


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

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

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
Member

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

@JIoJIaJIu good stuff, see some comments please!

@jorgeorpinel
Copy link
Contributor

Please let us know when iterative/dvc/pull/3246 is merged to check this doc vs the final behavior of the command πŸ™‚

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

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

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

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

- `-R`, `--recursive` - Recursively traverse the repo.
Copy link
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
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
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
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 iterative/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
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 iterative/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.

3 participants