Skip to content

Conversation

@jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Jan 13, 2020

Close #3092

@efiop efiop requested review from a user and Suor January 14, 2020 08:17
"""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.

Copy link
Contributor

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

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

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

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

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.

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.

Extracted docstring format discussion fully to #3176 (comment)

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.

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.

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

One comment

"""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`.
Copy link
Contributor

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.

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

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

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.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I'm going to say yes. 😅
Thanks, @jorgeorpinel

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel marked this pull request as ready for review January 17, 2020 23:44
@jorgeorpinel jorgeorpinel requested a review from pared January 17, 2020 23:44
@jorgeorpinel

This comment has been minimized.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thanks @jorgeorpinel !

@efiop efiop merged commit 74c5af1 into treeverse:master Jan 19, 2020
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.

api: inline documentation (docstrings)

5 participants