Adjust default option based on the --style

[leandro-lucarella-frequenz]

Now that google-style is also supported, it would be good if other options are adjusted to what is valid for google-style. For example:

• --allow-init-docstring should default to True
• --check-type-hint should default to False (ideally it should be checked if the function has type hints in the signature, then type hints can be omitted in the docstring, otherwise it should be present)

[jsh9]

I'm very hesitant to change the defaults, because if I flip some defaults for your use case today, there will be other users who open issues and ask me to flip them back.

What I recommend instead is to use a config file (see this section in the documentation) and define the options that you need.

As you can see, being author of pydoclint, even I need to configure something when pydoclint is linting itself:

pydoclint/pyproject.toml

Lines 9 to 12 in c0e69fd

	[tool.pydoclint]
	style = 'numpy'
	exclude = '\.git|\.tox|tests/data|unparser\.py'
	require-return-section-when-returning-none = true

[jsh9]

ideally it should be checked if the function has type hints in the signature, then type hints can be omitted in the docstring, otherwise it should be present

I think this is already the case today (v0.0.6).

I just tested on this code example, and pydoclint doesn't complain:

def myfunc(arg1, arg2, arg3):
    """
    Do something

    Args:
        arg1: Arg 1
        arg2: Arg 2
        arg3: Arg 3
    """
    print(123)

[leandro-lucarella-frequenz]

I'm very hesitant to change the defaults, because if I flip some defaults for your use case today, there will be other users who open issues and ask me to flip them back.

I don't agree with this, I think if I use google-style it is very confusing that correctly formatted google-style comments produce errors. Clearly people using non-standard google-style should pass special customization options themselves. Also this is a very common pattern in other linting tools that can adjust to one or other style, what that usually means is just setting a group of options automatically for you.

Of course I can go and configure the options I need and forget about it, but project-wise I think it is a mistake not to adjust the defaults because it will just make the life of new users more complicated for no reason.

I do see how changing defaults can have nasty effects on existing users, as it is effectively a breaking change, but since this project is so young, I think that should not be a problem.

[jsh9]

Hi @leandro-lucarella-frequenz , I did quite a bit of research on the topic of whether to allow __init__() docstrings by default. And let me show you what I found:

1. Google's docstring style guide requires a class docstring, but it's vague on whether there should be an __init__() docstring

2. On the other hand, Sphinx's Google style guide is quite strict. It says:

   The __init__ method may be documented in either the class level docstring, or as a docstring on the __init__ method itself.

   Either form is acceptable, but the two should not be mixed. Choose one convention to document the __init__ method and be consistent with it.

3. Despite Sphinx saying that "either form is acceptable", I think Sphinx slightly prefers people to document __init__() in the class docstring, because Sphinx always renders the class docstring by default, but users need a special directive (:special-members: __init__, under .. automodule::) to have the __init__() docstring rendered.

4. As of today, Sphinx is a lot more popular than MkDocs, which means more people are already used to Sphinx's convention

   • Sphinx has 9 million monthly downloads: https://pepy.tech/project/sphinx
   • MkDocs has 1 million monthly downloads: https://pepy.tech/project/mkdocs

Given the 4 points above, I think what I plan to do is the following:

• Remove the CLI option --allow-init-docstring
• Add a new CLI option: --document-init-method-in, which takes 3 values: "class", "init", and "both". I'll set "class" to be the default (because of No. 2 and No. 3 above).

[jsh9]

In that case I would expect the tool to check that either the code or the docstring have a type hint, and if both have a type hint, that they match. But maybe it should be a separate option, like --must-have-type-hints or something for checking that at least one of the code or docstring have type info.

Currently, pydoclint already can do this: "if both have a type hint, that they match".

However, pydoclint does not do --must-have-type-hints checking, and that is by design.

My philosophy is: pydoclint should not perform duplicated checks.

In this case, the presence of type hints can be (and should better be) checked by mypy.

Similarly, pydoclint doesn't check the presence of a docstring (because this should be handled by pydocstyle).

(Disclaimer: I'm well aware that pydoclint hasn't introduced mypy yet 😂. I opted for moving fast in the early stage, and I do plan to add mypy checks in pydoclint soon.)

[jsh9]

And let me address this issue that you raised:

luca@frq-lap-019:/tmp/venv [venv] 1 ! $ cat t.py 
def f(a: int) -> None:
    """This is a function.

    Args:
        a: This is an argument.
    """
    pass
luca@frq-lap-019:/tmp/venv [venv] $ pydoclint --style google t.py 
Skipping files that match this pattern: \.git|\.tox
t.py
t.py
    1: DOC105: Function `f`: Argument names match, but type hints do not match 

Seems to fail only when --style google is used.

This is an expected behavior and here is why:

If you set style to "numpy", the docstring parser doesn't recognize what Args: is, so the parser treats everything under Args: as normal texts.

This essentially makes the whole docstring a "short docstring" (i.e., a docstring with only text descriptions). By default, pydoclint doesn't check short docstrings (the --skip-checking-short-docstrings, or -scsd, option is True by default).

So if you flip this option:

pydoclint --style=numpy -scsd=False script.py

you'll see violations:

script.py
    1: DOC101: Function `f`: Docstring contains fewer arguments than in function signature.
    1: DOC103: Function `f`: Docstring arguments are different from function arguments. (Or could be other formatting issues: https://github.com/jsh9/pydoclint#notes-on-doc103 ). Arguments in the function signature but not in the docstring: [a: int].

Do you think I should flip the default of --skip-checking-short-docstrings to False? I'm now seeing how this default can be confusing to users.

[leandro-lucarella-frequenz]

Hi @leandro-lucarella-frequenz , I did quite a bit of research on the topic of whether to allow init() docstrings by default. And let me show you what I found

Just for the records, I'm not saying to make it a global default, I'm saying once you parse the --style google option, then you should change some options, which could be overridden again by parsing the following options in the CLI. So basically changing the defaults on a per-style basis.

Google's docstring style guide requires a class docstring, but it's vague on whether there should be an init() docstring

I really don't see how it is vague. I'm not saying it *should be, but it may be, it is allowed. That's pretty clear to me because the example exactly in the section you linked have the __init__ documented.

On the other hand, Sphinx's Google style guide is quite strict. It says:

To be honest I think then Sphinx is not really following Google style, but I can see your concern there. mkdocstrings allows you to control if you want to merge class and init docs or not, so I think it follows more correctly the google-style documentation.

That said.....

4. As of today, Sphinx is a lot more popular than MkDocs, which means more people are already used to Sphinx's convention

    * Sphinx has 9 million monthly downloads: [pepy.tech/project/sphinx](https://pepy.tech/project/sphinx)
    * MkDocs has 1 million monthly downloads: [pepy.tech/project/mkdocs](https://pepy.tech/project/mkdocs)
   

This is a good point.

Given the 4 points above, I think what I plan to do is the following:

* Remove the CLI option `--allow-init-docstring`

* Add a new CLI option: `--document-init-method-in`, which takes 3 values: "class", "init", and "both".  I'll set "class" to be the default (because of No. 2 and No. 3 above).

And this seems like a very reasonable compromise. 👍

[jsh9]

Yeah, can you create a new issue to discuss the --skip-checking-short-docstrings topic? That would make the discussions cleaner. Thanks!

[leandro-lucarella-frequenz]

Do you think I should flip the default of --skip-checking-short-docstrings to False? I'm now seeing how this default can be confusing to users.

I'm undecided about the default, I don't think it is well specified in the style guide, but I'm very happy that you can turn this on in pydoclint because we chose to make mandatory to document everything, so we would like to be able to enforce that.

That said, I still believe that not allowing to skip specifying types in the Args: section is a bug and unrelated to the --skip-checking-short-docstrings flag.

[jsh9]

Let me clarify my point on mypy a bit more:

My philosophy is: if a user wants to ensure type hints in function signatures, use mypy. And if a user also wants to ensure docstrings have matching type hints, also use pydoclint.

If, on the other hand, a user doesn't want to ensure type hints in function signatures, pydoclint will not complain about the absence of them. After all, this tool is named "pydoclint". If it also checks function signatures, it seems a bit out of the scope to me?

I think at some point, I'll need to add a paragraph or two in the readme to explain these details.

[jsh9]

I think I'll flip the default of --skip-checking-short-docstrings to False.

This way, the philosophy of pydoclint is more consistent: the default options are the most strict, and users can read the documentation to relax some options.

[jsh9]

Let's continue our discussion in #19 then.

[jsh9]

Hi @leandro-lucarella-frequenz , with #19 closed, have we addressed all the topics in this issue? If so, I think we can close this one?

[jsh9]

Closing this issue. No actions needed at this point.

(If I closed this issue by accident, please feel free to leave a comment here.)