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

[google] Adding types to the documentation should not be enforced #19

Closed
leandro-lucarella-frequenz opened this issue Jun 2, 2023 · 12 comments · Fixed by #25 or #29
Closed

[google] Adding types to the documentation should not be enforced #19

leandro-lucarella-frequenz opened this issue Jun 2, 2023 · 12 comments · Fixed by #25 or #29

Comments

@leandro-lucarella-frequenz
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 

This should not fail when using google-style, as it is allowed:

The description should include required type(s) if the code does not contain a corresponding type annotation.

https://google.github.io/styleguide/pyguide.html#doc-function-args
Originally posted by @leandro-lucarella-frequenz in #14 (comment)

@jsh9
Copy link
Owner

jsh9 commented Jun 2, 2023

Interesting. I wasn't quite familiar with the Google docstring style before.

So if we use the Google style, we should either have type hints in the signature, or in the docstring, but never both. Is my understanding correct?

I can already think of edge cases where people intentionally or unintentionally abuse this guide:

def myFunc(arg1, arg2: int, arg3: str) -> bool:
    """
    Do something

    Args:
        arg1 (str): Arg 1
        arg2: Arg 2
        arg3: Arg 3
            
    Returns:
        True or False
    """
    return True

How do you think pydoclint should handle this case above?

@leandro-lucarella-frequenz
Copy link
Author

So if we use the Google style, we should either have type hints in the signature, or in the docstring, but never both. Is my understanding correct?

I'm not sure if it is clearly defined but my understanding is that at least in one place is required, but you could have both too (and in this case it would be good for this tool to check that).

@leandro-lucarella-frequenz
Copy link
Author

How do you think pydoclint should handle this case above?

From the style guide I would think that's valid and this tool should should not raise any errors, but I think it is a obscure-enough case to not prioritize supporting the mixed style if it is too time consuming.

@leandro-lucarella-frequenz
Copy link
Author

There could be options to enforce one or the other style (or (dis)allowing the mixed style). This these tools I think there are so many combinations of other tools check or documentation generation tools that handle things differently that it would be impossible to comply with all without having a very fine-grained way to change what's allow and what is not, but again, I would optimize for the most common cases first :)

@jsh9
Copy link
Owner

jsh9 commented Jun 2, 2023

Assuming a user set --style=google, here's what I have in mind:

Add a new command-line option, --google-style-type-hints, which is only effective when --style=google.

And the existing --check-type-hints option should have no effects when --style=google.

--google-style-type-hints should take 4 values:

  • both
  • signature
  • docstring
  • neither

And here are the possible behaviors and the expected violations:

  • --google-style-type-hints=both
    • Violation when type hints don't strictly match
  • --google-style-type-hints=signature
    • Violation when there are no type hints in function signatures
    • Violation when there are any type hints in the docstring "Args" sections
  • --google-style-type-hints=docstring
    • Violation when there are any type hints in function signatures
    • Violation when there are no type hints in the docstring "Args" sections
  • --google-style-type-hints=neither
    • No violations

What do you think?

@jsh9
Copy link
Owner

jsh9 commented Jun 2, 2023

Also, I plan to let this case fail all 4 scenarios:

def myFunc(arg1, arg2: int, arg3: str) -> bool:
    """
    Do something

    Args:
        arg1 (str): Arg 1
        arg2: Arg 2
        arg3: Arg 3
            
    Returns:
        True or False
    """
    return True

Because I personally think this is bad practice. We should either put type hints all in the signature, all in the docstring, or in both. Mixing them is hard to maintain.

@leandro-lucarella-frequenz
Copy link
Author

Assuming a user set --style=google, here's what I have in mind:

Add a new command-line option, --google-style-type-hints, which is only effective when --style=google.

And the existing --check-type-hints option should have no effects when --style=google.

I don' t remember what the options for ˋ--check-type-hintsˋ where but I think it complicates the interface (and maybe the implementation too) to have 2 different options and make options take no effect depending on other options. I would only have one option and make it work for all styles. The, as I said in #14, I would change the defaults based on the chosen style to match that style.

--google-style-type-hints should take 4 values:

  • both
  • signature
  • docstring
  • neither

And here are the possible behaviors and the expected violations:

  • --google-style-type-hints=both
    • Violation when type hints don't strictly match

And what happens when functions have types and not docs or viceversa?

  • --google-style-type-hints=signature
    • Violation when there are no type hints in function signatures

I think checking only that signatures have type hints is ˋmypyˋ job, I wouldn't add an option for that according to your philosophy :).

  • Violation when there are any type hints in the docstring "Args" sections

I would call this option signatures-only then.

  • --google-style-type-hints=docstring
    • Violation when there are any type hints in function signatures

Sounds weird, but OK.

  • Violation when there are no type hints in the docstring "Args" sections

I consequentially would call this docstring-only

  • --google-style-type-hints=neither
    • No violations

What do you think?

I don' t find that scheme all that useful to be honest. IMHO the most common scenarios probably are:

  • If there are type hints in both code and docs, then they should always match. I can't think of any cases where some would write type hints in both and purposely would want them not to match if they are running a doc linting tool.
  • Have an option to switch on and off forcing users to document type hints. If checking is on, then it should complain if the docstrings don' t have types. If is off it should not complain if type hints are missing in the docstrings. This option should not care about type hints in the code.
  • To better support google-style there could be another option to enforce having type hints in either the code or the docs. If this option is on, it should make sure that every argument has type info either in the code or docs (and as per the first item that both match if it has both). If this is implemented I would support the mixed style, at I don't think I'm an authority to determine what is good or bad practice.

IMHO the last option is a pretty obscure use case already, I think the google style guide was written when type hints were new. By now if you want to specify type hints I see barely any good reason to do it only in the docs (maybe support very old python versions, but even then you can use ˋ# type: ˋ annotations. In an ideal world I would support it, because again I don' t feel I'm anybody to decide what is good or bad, is just that I think it is rare enough to maybe not be worth the effort (at least until somebody asks for it).

Hope you find these insights useful! :)

@jsh9
Copy link
Owner

jsh9 commented Jun 5, 2023

Thanks for the suggestions!

I came up with a 2nd version:

  • --type-hints-in-signature (True or False)
    • If True, report a violation when type hints are missing from the function signature
    • If False, report a violation when there are type hints in the function signature (input arguments only)
  • --type-hints-in-docstring (True or False)
    • If True, report a violation when type hints are missing from the docstring
    • If False, report a violation when there are type hints in the function signature (input arguments only)

I know I mentioned that it's mypy's job to check type hints in the signature, but this 2nd version is easier to understand (especially for those who are not familiar with mypy).

Naturally, when both options are True, the type hints need to match.

And for your code bases, you can set one of the option to True, and the other to False.

Since the default style is numpy, the defaults of these 2 new options will be True and True.

What do you think of this version?

@leandro-lucarella-frequenz
Copy link
Author

Hi @jsh9, you are welcome.

I think your newly proposed scheme still forbids valid google-style combinations, you could potentially documents types in the code for one function and in the docs for some other, and in both for a third. If this were my project, as I said before I would go for a more flexible and less pedantic/strict set of options to allow more flexibility to the user, but your proposed flags covers our use case (--type-hints-in-signature=True, which will overlap 100% with mypy so we'll be effectively checking this twice in our case and --type-hints-in-docstring=False).

For the records, I can see a use case for more flexibility. For example, we started using documentation in the docstrings and the code, but then migrated to only do it in the code. This took a while and for some time the code had mixed styles. Of course doing all in one go is also possible but there might be projects with big code bases where this might not be trivial and might put extra difficulties to adopt pydoclint earlier. But again, as long as nobody is asking for it, I guess it is not high priority.

About defaults I leave the discussion for #14 :)

@jsh9
Copy link
Owner

jsh9 commented Jun 6, 2023

Hi @leandro-lucarella-frequenz , yes, there are different levels of "homogeneity":

  1. Most homogeneous: every file in a project either all put type hints in the docstring, or in the signature
  2. Somewhat homogeneous: some files put type hints in the docstring; some put type hints in the signature
  3. Heterogeneous: in the same file, some functions have type hints in the docstring, and some have type hints in the signature
  4. Chaotic: in the same functions, some arguments have type hints in the docstring, and some other arguments have type hints in the signature

I think it's better for linters to promote the first case because that's best practice. And I wouldn't want pydoclint to ever "indulge" the 3rd and the 4th case.

You mentioned Case 2 in your comment above, and I think that's a valid use case. So here's what I propose:

pydoclint --type-hints-in-signature=True --type-hints-in-docstring=False file01.py file02.py ... file10.py
pydoclint --type-hints-in-signature=False --type-hints-in-docstring=True file11.py file12.py ... file20.py
pydoclint --type-hints-in-signature=True --type-hints-in-docstring=True file21.py file22.py ... file30.py
pydoclint --type-hints-in-signature=False --type-hints-in-docstring=False file31.py file32.py ... file40.py

This would provide people a transition period to gradually update the code styles.

@jsh9
Copy link
Owner

jsh9 commented Jun 12, 2023

Hi @leandro-lucarella-frequenz : I implemented the new options. The changes are included in v0.0.10.

I tried v0.0.10 on your code base "frequenz-channels-python" with the following options:

  • --style=google
  • --type-hints-in-docstring=False
  • --type-hints-in-signature=True
  • --allow-init-docstring=True

And I indeed discovered a place where the type hints are flipped:

src/frequenz/channels/util/_example.py
    18: DOC106: Method `ExampleClass.__init__`: The option `--type-hints-in-signature` is `True` but there are no type hints in the signature
    18: DOC107: Method `ExampleClass.__init__`: The option `--type-hints-in-signature` is `True` but not all args in the signature have type hints
    18: DOC111: Method `ExampleClass.__init__`: The option `--type-hints-in-docstring` is `False` but there are type hints in the docstring arg list
    18: DOC103: Method `ExampleClass.__init__`: 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: [param1: , param2: , param3: ]. Arguments in the docstring but not in the function signature: [param1: str, param2 (: , param3: list(str)].
    66: DOC106: Method `ExampleClass.example_method`: The option `--type-hints-in-signature` is `True` but there are no type hints in the signature
    66: DOC107: Method `ExampleClass.example_method`: The option `--type-hints-in-signature` is `True` but not all args in the signature have type hints

@llucax
Copy link
Contributor

llucax commented Aug 7, 2023

This worked great, thanks!

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 a pull request may close this issue.

3 participants