api: docstring updates

[jorgeorpinel]

Matches iterative/dvc.org/pull/908

• ❗ Have you followed the guidelines in the Contributing to DVC list?

• 📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

• ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[efiop]

Do we really need this? It is a private helper consisting of 2 lines :)

[jorgeorpinel]

Maybe not but per some recent chat with @shcheklein I thought we want to always mention exceptions in docstrings for user-facing modules at least (its possible an IDE might show this docstring to a user). I can easily remove it if I'm confused.

[jorgeorpinel]

Or mention it in the docstring of the corresponding public functions that use this private one?

[efiop]

@jorgeorpinel So there is a proper format for docstrings to mark that something raises something. The info about UrlNotDvcRepoError should be in public methods. E.g. in google docstrings:

   Raises:
        UrlNotDvcRepoError: If repo is not a DVC repository.

I don't think any IDE will be able to figure out what some method rises based on private methods docstrings that it uses 🙂

[efiop]

I would remove this one for now.

[jorgeorpinel]

Thanks for finishing this PR for me @efiop!

there is a proper format for docstrings to mark that something raises something... E.g. in google docstrings

Confused 😕 I thought we decided NOT to use any fancy formats? See #3176 (comment)

yes, the idea was too mention in the public method description

OK @shcheklein so should we add this one to get_url? I can send a follow-up PR for that. Just not sure what (lack of) docstring format to use...

dvc/dvc/api.py

Lines 20 to 27 in f673144

	def get_url(path, repo=None, rev=None, remote=None):
	"""Returns URL to the storage location of a data artifact tracked
	by DVC, specified by its path in a repo.
	NOTE: There's no guarantee that the file actually exists in that location.
	"""
	with _make_repo(repo, rev=rev) as _repo:
	_require_dvc(_repo)

[shcheklein]

@jorgeorpinel could we check how stdlib deals with it in case they mention important exception?

[jorgeorpinel]

Checking .../3.7/lib/python3.7/os.py (os module), I see that it just casually mentions exceptions are raised (not even with exact types) in docstrings:

def makedirs(name, mode=0o777, exist_ok=False):
    """makedirs(name [, mode=0o777][, exist_ok=False])

    ... If the target directory already
    exists, raise an OSError if exist_ok is False. Otherwise no exception is
    raised.

def walk(top, topdown=True, onerror=None, followlinks=False):
    """Directory tree generator.

    ... It can
    report the error to continue with the walk, or raise the exception
    to abort the walk.

def _fspath(path):
    """Return the path representation of a path-like object.

    ... If the
    path representation is not str or bytes, TypeError is raised. If the
    provided path is not str, bytes, or os.PathLike, TypeError is raised.

Note that not all raised exceptions raised in public functions are documented in docstrings (and _fspath is not even public – seems arbitrary), for example get_exec_path() can raise a ValueError (directly) but this is not mentioned in its docstring (maybe because it's a pretty generic/obvious exception). popen() can also raise TypeError and ValueError exceptions (not mentioned in docstring), same in fdopen() – probably because they're near the top of each fn so it's obvious/self-documenting.

[shcheklein]

so, sound like a pretty reasonable approach in this case, especially considering that we are taking stdlib as an inspiration for the format in general. What do you think?

[efiop]

Note that not all raised exceptions raised in public functions are documented in docstrings (and _fspath is not even public – seems arbitrary)

This doesn't apply to the case we've described here, as that docstring was pointless, it was commenting on 2 lines of obvious code with an obvious comment :)

Confused confused I thought we decided NOT to use any fancy formats? See #3176 (comment)

Sorry, that discussion went by me. Left a comment there. But my comment in this PR was simply stating that obvious docstrings in private methods are not that useful as is and are not even useful in auto-parsed scenarios.