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

docsig doesn't seem to support @typing.overload-ed functions/methods well #196

Closed
EFord36 opened this issue Nov 10, 2023 · 2 comments · Fixed by #205
Closed

docsig doesn't seem to support @typing.overload-ed functions/methods well #196

EFord36 opened this issue Nov 10, 2023 · 2 comments · Fixed by #205
Assignees

Comments

@EFord36
Copy link
Contributor

EFord36 commented Nov 10, 2023

Hi,

Consider this code, modified from the typing docs to include a docstring:

from typing import overload

@overload
def process(response: None) -> None:
    ...
@overload
def process(response: int) -> tuple[int, str]:
    ...
@overload
def process(response: bytes) -> str:
    ...
def process(response):
    """process a response.

    :param response: the response to process
    :return: something depending on what the response is
    """
    ...  # actual implementation goes here
docsig checking code

from docsig import docsig
    
string_to_check = """
from typing import overload

@overload
def process(response: None) -> None:
    ...
@overload
def process(response: int) -> tuple[int, str]:
    ...
@overload
def process(response: bytes) -> str:
    ...
def process(response):
    \"\"\"process a response.

    :param response: the response to process
    :return: something depending on what the response is
    \"\"\"
    ...  # actual implementation goes here
"""
docsig(string=string_to_check)

This currently throws a bunch of docsig errors:

4
-
def process(✖response) -> ✓None:
    ...

E113: function is missing a docstring

7
-
def process(✖response) -> ✖tuple[None]:
    ...

E113: function is missing a docstring

10
--
def process(✖response) -> ✖str:
    ...

E113: function is missing a docstring

13
--
def process(✓response)?:
    """
    :param response: ✓
    :return: ✖
    """

E109: cannot determine whether a return statement should exist or not

But to my mind this is documented in a way that seems reasonable.

The fact that Sphinx supports this for rendering docstrings with autodoc/autosummary makes me think this is do-able (their code for supporting this is here), but it seems like this may significantly increase the complexity of docsig's docstring parsing code.

A couple of ways of opting out of supporting this for docsig:

  1. Document it as unsupported, don't change the default behaviour but allow comments like '# docsig off' that turn off docsig checks for a function
  2. Turn off all checks for overload-ed functions/methods, and document this. However, this still requires figuring out which functions/methods are overload-ed, which is still potentially a reasonable amount of work/change to the existing logic.
@jshwi
Copy link
Owner

jshwi commented Nov 12, 2023

Hi @EFord36 ,

Thanks for spotting this, and thanks for the suggestions

Definitely looks doable, I'd welcome a PR, but I'll also have a look at this and see what I can do

Ideally I'd really like to be able to disable checks with comment directives. I haven't been able to pull it off yet, as far as I can remember It looked a whole lot harder to do with an ast than it is to do when working with a tokeniser. Seems I've forgotten about implementing this, but I'm going to have another look.

Cheers!
Stephen

@jshwi jshwi self-assigned this Nov 14, 2023
jshwi added a commit that referenced this issue Nov 14, 2023
Signed-off-by: Stephen Whitlock <stephen@jshwisolutions.com>
@jshwi jshwi linked a pull request Nov 14, 2023 that will close this issue
@jshwi
Copy link
Owner

jshwi commented Nov 14, 2023

@EFord36
I've implemented an overload check, and I'll push another commit for a disable comments feature
Can't say it'll be perfect, but it's pretty well tested
If it doesn't behave as expected I'd really appreciate if you opened another issue, any time
Thanks mate!
Stephen

@EFord36 EFord36 changed the title docsign doesn't seem to support @typing.overload-ed functions/methods well docsig doesn't seem to support @typing.overload-ed functions/methods well Nov 14, 2023
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.

2 participants