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

Add a Raises: section in docstrings where needed #192

Open
LaurentAjdnik opened this issue Jun 7, 2021 · 8 comments
Open

Add a Raises: section in docstrings where needed #192

LaurentAjdnik opened this issue Jun 7, 2021 · 8 comments
Labels
archive Smthg to keep in mind, but not worth implementing now style Style and format related issues

Comments

@LaurentAjdnik
Copy link
Collaborator

As specified in https://google.github.io/styleguide/pyguide.html#doc-function-raises.

Only two functions (in simresults.py) currently have such a section, whereas so many of them can raise Errors.

@LaurentAjdnik
Copy link
Collaborator Author

BTW, shouldn't we add https://pypi.org/project/flake8-docstrings/ to CI ?

I tested it locally and the results were... kinda wild...

@HGSilveri
Copy link
Collaborator

As specified in https://google.github.io/styleguide/pyguide.html#doc-function-raises.

Only two functions (in simresults.py) currently have such a section, whereas so many of them can raise Errors.

@LaurentAjdnik , note the last sentence of that link you sent me:

You should not document exceptions that get raised if the API specified in the docstring is violated (because this would paradoxically make behavior under violation of the API part of the API).

Most errors we raise come exactly from violations of the API specified in the docstring, which is why they are not documented in the docstrings. Nonetheless, there can still be raised errors out there that should be documented and that we missed - if you find any of those, feel free to open a PR adding the respective Raises section.

@HGSilveri
Copy link
Collaborator

BTW, shouldn't we add https://pypi.org/project/flake8-docstrings/ to CI ?

I tested it locally and the results were... kinda wild...

This is a great idea, though it needs some fine-tuning. I'll open a separate issue on it.

@LaurentAjdnik
Copy link
Collaborator Author

Note the last sentence of that link you sent me:

You should not document exceptions that get raised if the API specified in the docstring is violated (because this would paradoxically make behavior under violation of the API part of the API).

Honestly, this sentence does not make much sense to me. What exactly is a "violation of the API" ? And what is not ?

Most errors we raise come exactly from violations of the API specified in the docstring, which is why they are not documented in the docstrings.

While working on #157, I counted 92 Errors raised throughout the code.

From the list I pasted in that issue, can you give me a few examples of what would be, with certainty:

  • "Violations of the API"?
  • "Non-violations of the API" hence requiring docstrings?

Right now, I feel like pretty much everything, except TypeErrors, could/should be included in docstrings... 😁

@HGSilveri
Copy link
Collaborator

Strictly speaking you're right. Every time we raise a ValueError because a value is, for example, out of range, and we did not specify this range in the docstring, then it's not a violation of the API.

However, there are times when the range is implicit (e.g. when we ask for a duration, we don't specify it should be a positive number, but it's obvious it should), and I think in cases like this, specifying the potential ValueErrors bloats the docstring unnecessarily.

So yeah, I would agree with you that generally, outside of TypeErrors, we don't follow this rule to the tee, but do so when it reasonable.

That being said, if you feel the need to attach a Raises: ValueError because of a threshold error, for example, I would rather have the threshold specified in the definition of the variable instead.

@HGSilveri
Copy link
Collaborator

@LaurentAjdnik is it okay if I close this issue?

@LaurentAjdnik
Copy link
Collaborator Author

BTW, shouldn't we add https://pypi.org/project/flake8-docstrings/ to CI ?
I tested it locally and the results were... kinda wild...

This is a great idea, though it needs some fine-tuning. I'll open a separate issue on it.

@HGSilveri: Shall we open this specific issue? I agree there would be some (= a lot of?...) fine-tuning before the results are satisfying.

More generally, I think there are places where we could/should add somes Raises: in the docstrings but I haven't really thought about it yet.

@HGSilveri
Copy link
Collaborator

@HGSilveri: Shall we open this specific issue? I agree there would be some (= a lot of?...) fine-tuning before the results are satisfying.

@LaurentAjdnik Actually, I already took care of that one (see #193 and #194).

More generally, I think there are places where we could/should add somes Raises: in the docstrings but I haven't really thought about it yet.

Alright, I'll leave it open. Do you want me to assign you then?

@HGSilveri HGSilveri added the style Style and format related issues label Feb 23, 2022
@HGSilveri HGSilveri added the archive Smthg to keep in mind, but not worth implementing now label May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archive Smthg to keep in mind, but not worth implementing now style Style and format related issues
Projects
None yet
Development

No branches or pull requests

2 participants