-
Notifications
You must be signed in to change notification settings - Fork 15
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
Optional arguments false positive #30
Comments
Hi @ghjklw, I understand your point. However, I am a bit concerned that this could become a slippery slope. For example, if pydoclint allows It is possible that, with lots of bespoke logic, pydoclint can support all these variations, but I'm also a bit concerned that parsing these variations will make pydoclint a lot slower. |
Another example to show that things could become intractable is a type hint like this:
To "translate" this to pseudo natural language, it would have to be: "int or float or (bool, optional) or list of str, optional" 😅 |
Well, this could be simplified to int | float | bool | list[str] | None Which could be written in pseudo natural language like this:
While I think pydoclint should support the syntaxes for optional arguments as defined in the official specifications, I don't think it should bother to support things like |
First of all, I checked the official Google Python style guide, and I didn't see Google recommending/enforcing any guidance for writing type hints in docstrings. The example shown in the Sphinx documentation is Sphinx's style ("str, optional"). It's a fine style, but not quite practical for a linter. There is only one way to specify type before Python 3.10, and a new way since Python 3.10. This standardization simplifies people's lives. At least, I personally only need to remember that "docstring's type hint should match the signature's type hint". On the contrary, allowing natural languages can introduce ambiguities, variations, and people forgetting what the rules are. Let me give you another example:
Shall we allow |
Hi! I now realise my initial comment was a bit misleading! I actually didn't understand I thought this was a part of the Google format, but you're right, it isn't. However, it is part of Numpy's style guide:
In other words, the logic here could be quite simple: if the function signature defines a default value, then the type should either contain I couldn't however find a good example to determine whether the type in the docstring should be |
Putting aside the question of the natural language representation of types (which I think, if desirable, should be tracked as an independent issue), this raises another interesting point: from typing import Optional
def example1(var: str | None) -> None:
"""Test function 1.
Args:
var (str | None): Variable description.
"""
def example2(var: str | None) -> None:
"""Test function 2.
Args:
var (str|None): Variable description.
"""
def example3(var: Optional[str]) -> None:
"""Test function 3.
Args:
var (str | None): Variable description.
""" Raises the following validation errors:
I guess that's because types are compared as strings, but it is also possible to do: >>> (str | None) == (str|None)
True
>>> (str | None) == Optional[str]
True
>>> Optional[Union[int, float, Optional[bool], List[str]]] == (int | float | bool | List[str] | None)
True Wouldn't that be a better way of comparing types? Do you want to create another feature request to discuss it? |
It definitely be a desirable feature but I imagine it could be quite difficult to implement. |
Interesting! I didn't know that we can directly compare type hints ( @ghjklw you are correct that the comparison of types in pydoclint is currently done via string comparison. Here's a code snippet: pydoclint/pydoclint/utils/arg.py Lines 31 to 35 in 1a3596f
pydoclint only does static syntax analysis (more in what I mentioned in this comment: #35 (comment)). Doing "dynamic" code analysis on the full source code is most likely why darglint is slow. That said, dynamically parsing type hint strings may not be that slow. This is something I can do right now: exec('left = str | None')
exec('right = str|None')
assert left == right However, I don't know how to handle cases where we need quotes in type hints. For example, the argument type is a custom class that needs to be quoted. Any suggestions or ideas? |
What do you mean by that? No type hints have to be quoted and why would that be a problem? |
See this example: pydoclint/pydoclint/utils/arg.py Line 37 in 1a3596f
|
|
Putting aside the security risk of using |
Oh wow, I didn't know about Naturally, there will be people like me who don't know about I agree with @ghjklw 's comment above; there may be too many edge cases that we can't possibly think of. That said, I wonder whether we can compare 2 string when ignoring any spaces? If we remove any spaces from both strings and they are the same, can we consider them to represent the same type? |
Thinking about it I only see the following cases where it might make sense to have a docstring type that doesn't match the type hint.
|
The type system will use
I don't know of a type hint that doesn't separate two types by either |
A bit hacky, but can probably be improved and certainly more robust/generic than just ignoring spaces: ast.dump(ast.parse("str|None")) == ast.dump(ast.parse("str | None")) |
I already have a method to remove quotes from the type hint: pydoclint/pydoclint/utils/arg.py Lines 96 to 98 in 1a3596f
And I think using With
But |
Hi @ghjklw , I forgot to address your previous point above
I used to write And now in pydoclint, I'm actually "inheriting" that behavior from darglint. I think there are 2 new features that I can potentially add:
No. 2 is a slippery slope, because rule-based natural language parsers are never easy to implement. Too many edge cases. I can already think of some:
|
Update: I haven't got a chance to get to it yet. But this issue is still on my mind. |
Hi @ghjklw , I found that this PR (#58) actually fixed the issue of comparing However, I still think that to consider Therefore, I'll label But if you have any additional comments or suggestions, please feel free to add comments below, or re-open this issue. Thanks! |
Both of these examples trigger an error
DOC105: Function test: Argument names match, but type hints do not match
:Changing the var1 type to
str | None
solves it, but this is a bit redundant and, as far as I know, inconsistent with how most other tools work. You can see for example the functionmodule_level_function
from Sphinx napoleon documentation.The text was updated successfully, but these errors were encountered: