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

Allow more control over--skip-checking-short-docstrings #112

Open
Tracked by #215
llucax opened this issue Jan 8, 2024 · 4 comments
Open
Tracked by #215

Allow more control over--skip-checking-short-docstrings #112

llucax opened this issue Jan 8, 2024 · 4 comments

Comments

@llucax
Copy link
Contributor

llucax commented Jan 8, 2024

The --skip-checking-short-docstrings is extremely convenient to keep documentation of simple methods very short and avoid redundancy, but it comes with a cost. When enabled, it can lead to a lot of unintended missing documentation.

It would be good to be able to have more control over what's accepted and what's not. For example:

  • Maximum amount of arguments a function can have omitting the Args section, not counting self (for example, a simple function, like a setter, with just 1 or 2 argument it is very likely properly documented when omitting Args, but a function with, say 3 or more is highly unlikely that's OK to skip the Args.

  • Option to still require Raises if the function raises exceptions and --skip-checking-short-docstrings is enabled. It is highly unlikely that's OK to not document raised exceptions. Alternatively it could take a list of exceptions that are OK to skip, if the list is empty, then no exceptions can be skipped (for example, it might be OK to omit ValueError, but not other exceptions).

  • Option to allow not documenting, or documenting only with a summary line, dunder methods (__str__, __repr__, __sum__, etc.). This is because these methods are well defined elsewhere, so probably documenting them is redundant in most cases. An exception list is also probably needed, and should default to __init__, __new__ and __init_subclass__ (am I missing anything else?), as these methods have variable arguments which are very important to document.

@jsh9
Copy link
Owner

jsh9 commented Jan 8, 2024

I like the 2nd and the 3rd idea, and I will find time to implement them.

For the 1st idea, I think it may cause the code logic and the user experience too complex. Specifically, right now, the only way to skip writing the Args section is to wrote a short docstring. But if we implement the 1st idea, there will be 2 ways to skip writing the Args section, and these 2 ways may interact with each other, making it more difficult for users to understand and configure.

@llucax
Copy link
Contributor Author

llucax commented Jan 9, 2024

Cool, thanks!

I can understand the goal of keeping things simple, maybe another option would be to remove --skip-checking-short-docstrings and split it into more fine-grained options (or keep --skip-checking-short-docstrings just as an alias for some of the more fine-grained options).

In the current state, at for us --skip-checking-short-docstrings is of very little use, because it is just too relaxed, and it is too easy to accidentally underdocument functions, defeating the whole purpose of having pydoclint in the first place.

The convenience of being able to skip redundant information is still very useful.

So what about something like:

  1. --allow-missing-args-for-short-docstrings N: Where N is an optional maximum number of arguments a function can have to be OK to skip Args in the docstring when a short string is used, if -1 is used there is no limit, Args will never be required for short docstrings. Ideally it defaults to 1 when the option is specified without an N, but it could be mandatory too if the argument passing doesn't support an optional argument to the option.

  2. --allow-missing-raises-for-short-docstrings E: Where E is an optional regex to match against which exceptions are OK to skip in the Raises section, with .* as default (so it is OK to skip any exception in Raises), so --skip-checking-raises-for-short-docstrings '^ValueError$' will not emit any errors if ValueError is is raised, but any other exception must be included in Raises.

3, --allow-missing-args-for-short-docstrings-in-dunder-methods: Args is not checked for dunder methods except for __init__, __new__ and __init_subclass__. For those exceptions, --skip-checking-args-for-short-docstrings applies, for the others --skip-checking-args-for-short-docstrings is ignored.

  1. --skip-checking-short-docstrings: Remove it or make it an alias for --allow-missing-args-for-short-docstrings '-1' --allow-missing-raises-for-short-docstrings '.*' --allow-missing-args-for-short-docstrings.

So there is a small overlap between 1 and 3, but it is still well defined and I think very useful. If no overlap is desired at all, then I think keeping 1 is actually more useful than 3, as 3 is more rare and for the most common cases using --skip-checking-raises-for-short-docstrings 2 should also cover for most (if not all) dunder methods.

All --allow-missing-xxx should still check any present sections for correctness.

For checking the Returns/Yield I think #110 and #111 should apply.

UPDATE: Changed options to use --allow-missing instead of --skip-checking.

@jsh9
Copy link
Owner

jsh9 commented Jan 29, 2024

Hi @llucax ,

Sorry for the delayed reply. I've had some time to rethink these feature requests that you made, and here is what I think now:

  • Config options such as --allow-missing-args-for-short-docstrings N can be too flaky. For example, someone sets N at 3, and a function with 3 arguments are not checked. Later, people add a few more arguments to this function, and suddenly pydoclint starts throwing violations. People will need to carefully investigate pydoclint configs to find that N is the culprit.
  • Similarly for --allow-missing-raises-for-short-docstrings E. People may suddenly find the pipeline failing if they add new exceptions to their functions. They'd also need to investigate pydoclint configs to find that E is the culprit.

On the contrary, it's natural to require all arguments and all exceptions to be properly checked, so people don't really need to remember any special configs (such as N or E).

@llucax
Copy link
Contributor Author

llucax commented Jan 31, 2024

Fair enough. Do you think the other part of this issue is still useful? I mean checking for missing Raises?

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

No branches or pull requests

2 participants