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

document dvcfs #8332

Merged
merged 1 commit into from
Sep 21, 2022
Merged

document dvcfs #8332

merged 1 commit into from
Sep 21, 2022

Conversation

skshetry
Copy link
Member

Only documenting DvcFileSystem.__init__ and isdvc for now, because others inherit the docstrings from superclass.

@skshetry skshetry added the skip-changelog Skips changelog label Sep 20, 2022
@skshetry skshetry requested a review from daavoo September 20, 2022 09:20
@skshetry skshetry self-assigned this Sep 20, 2022
@@ -238,7 +272,8 @@ def _open(
dvc_path = _get_dvc_path(dvc_fs, subkey)
return dvc_fs.open(dvc_path, mode=mode)

def isdvc(self, path, **kwargs):
def isdvc(self, path, **kwargs) -> bool:
"""Is this entry dvc-tracked?"""
Copy link
Member Author

@skshetry skshetry Sep 20, 2022

Choose a reason for hiding this comment

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

fsspec is styled in the same way, see fs.isfile and fs.isdir.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bit of a question on whether isdvc even means anything and if it is that useful. We can keep it for now as is, but there is a chance it will change in the future and that's probably alright.

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

I would also add a docstring to ls, as there is a discrepancy (dvc_only) with the fsspec signature

dvc/fs/dvc.py Outdated
Comment on lines 88 to 90
Defaults to the default branch in case of remote repositories.
In case of a local repository or if the repo is not a Git repo,
this option is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Defaults to the default branch in case of remote repositories.
In case of a local repository or if the repo is not a Git repo,
this option is ignored.
Defaults to the default branch in case of Git repositories.
In case the repo is not a Git repo this option is ignored.

@skshetry As far as I have tried, rev also works for local Git repositories, right?

@dberenbaum
Copy link
Collaborator

Fixes #8336

@dberenbaum
Copy link
Collaborator

Is there a way to auto generate docs for only dvcfs?

Copy link
Collaborator

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

@skshetry
Copy link
Member Author

I would also add a docstring to ls, as there is a discrepancy (dvc_only) with the fsspec signature

I am trying to avoid duplicating docstrings (AFAIK you cannot extend docstrings just for dvc_only).

@skshetry skshetry merged commit ba1ab86 into iterative:main Sep 21, 2022
@skshetry skshetry deleted the document-dvcfs branch September 21, 2022 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog Skips changelog
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants