Skip to content

api: better code documentation #3130

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 19, 2020
Merged

api: better code documentation #3130

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f652377
api: improve existing docstrings
jorgeorpinel Jan 13, 2020
4d05ad2
Merge branch 'master' of github.com:iterative/dvc
jorgeorpinel Jan 17, 2020
2b818e2
api: note get_url doesn't chekc file existence
jorgeorpinel Jan 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions dvc/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ def __init__(self, url):


def get_url(path, repo=None, rev=None, remote=None):
"""Returns an url of a resource specified by path in repo"""
"""
Returns the full URL to the data artifact specified by its `path` in a
`repo`.

This comment was marked as resolved.

Sign in to view

Copy link
Contributor

@Suor Suor Jan 16, 2020

Choose a reason for hiding this comment

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

I don't like backticks, we don't use them anywhere else. Python stdlib doesn't use them either.

Copy link
Contributor

@shcheklein shcheklein Jan 16, 2020

Choose a reason for hiding this comment

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

any suggestions? is there a standard way of doing markup that is understood by the tools that generate a view for this (like F1 in PyCharm, quick help in VS Code, docs generators, etc)

btw, we do use them as far as I can tell internally, also, Markdown markup or some markup is being used in other libraries for external APIs

Python stdlib:

Return whether an object is an instance of a class or of a subclass thereof.
    
    A tuple, as in ``isinstance(x, (A, B, ...))``, may be given as the target to
    check against. This is equivalent to ``isinstance(x, A) or isinstance(x, B)
    or ...`` etc.

double backtics to highlight the code (and probably make it linkable).

Am I missing something here?

Copy link

@ghost ghost Jan 17, 2020 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

@shcheklein , there are plenty docstring formats: https://stackoverflow.com/questions/3898572/what-is-the-standard-python-docstring-format

From PEP 287:

Text enclosed in single backquotes is recognized as "interpreted text", whose interpretation is application-dependent. In the context of a Python docstring, the default interpretation of interpreted text is as Python identifiers. The text will be marked up with a hyperlink connected to the documentation for the identifier given

Not sure if any particular IDE takes advantages of such feature.

we don't use them anywhere else

@Suor, we actually do 😅 but it is quite inconsistent:

grep -R '`.*`' dvc

I'd say that there's no need to block a PR for this. I created a separate issue to discuss the formatting across the code base: #3176

Copy link
Contributor

@shcheklein shcheklein Jan 17, 2020

Choose a reason for hiding this comment

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

Not sure if any particular IDE takes advantages of such feature.

yep, all of those I've used at least.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 18, 2020

Choose a reason for hiding this comment

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

Text enclosed in single backquotes is recognized as "interpreted text"... In the context of a Python docstring, the default interpretation of interpreted text is as Python identifiers. The text will be marked up with a hyperlink connected to the documentation for the identifier given

This is what I was going for here. So keep it for now guys? I don't initially have a strong opinion.

there a standard way of doing markup that is understood by the tools that generate a view for this (like F1 in PyCharm, quick help in VS Code, docs generators, etc)

Yeah great question @shcheklein! I'll look it up.

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 18, 2020

Choose a reason for hiding this comment

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

Extracted docstring format discussion fully to #3176 (comment)

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So to resolve this one @Suor, as most available formats use backquotes except Google's, are you able to approve this for the time being? We will revisit this once #3176 is resolved anyway.

NOTE: There is no guarantee that the file actually exists in that location.
"""
try:
with _make_repo(repo, rev=rev) as _repo:
abspath = os.path.join(_repo.root_dir, path)
Expand All @@ -67,7 +71,7 @@ def get_url(path, repo=None, rev=None, remote=None):


def open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
"""Opens a specified resource as a file descriptor"""
"""Context manager to open a file artifact as a file object."""
args = (path,)
kwargs = {
"repo": repo,
Expand All @@ -86,7 +90,7 @@ def __init__(self, func, args, kwds):

def __getattr__(self, name):
raise AttributeError(
"dvc.api.open() should be used in a with statement"
"dvc.api.open() should be used in a with statement."
)


Expand All @@ -100,7 +104,7 @@ def _open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):


def read(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
"""Read a specified resource into string"""
"""Returns the contents of a file artifact."""
with open(
path, repo=repo, rev=rev, remote=remote, mode=mode, encoding=encoding
) as fd:
Expand All @@ -110,15 +114,17 @@ def read(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
@contextmanager
def _make_repo(repo_url, rev=None):
if not repo_url or os.path.exists(repo_url):
assert rev is None, "Custom revision is not supported for local repo"
assert (
rev is None
), "Git revisions are not supported for local DVC projects."
Copy link

@ghost ghost Jan 17, 2020

Choose a reason for hiding this comment

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

Why not "local repositories"? Not sure I understand what's a local DVC project.

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Jan 17, 2020

Choose a reason for hiding this comment

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

A DVC project isn't always a repo. It can be initialized with --no-scm and you can provide the local path to that project in the file system to the repo argument (misleading name BTW) of API functions.

yield Repo(repo_url)
else:
with external_repo(url=repo_url, rev=rev) as repo:
yield repo


def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None):
"""Instantiate an object described in the summon file."""
"""Instantiate an object described in the `summon_file`."""
with prepare_summon(
name, repo=repo, rev=rev, summon_file=summon_file
) as desc:
Expand Down