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

Default argument values in overloaded functions should be taken from actual definition #8514

Closed
Jackenmen opened this issue Dec 2, 2020 · 4 comments
Labels
extensions:autodoc type:enhancement enhance or introduce a new feature
Milestone

Comments

@Jackenmen
Copy link

Jackenmen commented Dec 2, 2020

Is your feature request related to a problem? Please describe.
Currently, the default argument value in the overloaded signatures of the function is used rather than the actual default value that is used which is the default value from the actual function definition.

As an example, for this code the actual default value is None, not ... that is used there just as a placeholder (a common idiom when using @typing.overload):

    @overload
    async def get_shared_api_tokens(self, service_name: str = ...) -> Dict[str, str]:
        ...

    @overload
    async def get_shared_api_tokens(self, service_name: None = ...) -> Dict[str, Dict[str, str]]:
        ...

    async def get_shared_api_tokens(
        self, service_name: Optional[str] = None
    ) -> Union[Dict[str, Dict[str, str]], Dict[str, str]]:
        """Some doc"""
        # actual implementation here

And here's how it's rendered by Sphinx:
image

Describe the solution you'd like
I would want the function signatures to show the default value used in the actual function definition.

Describe alternatives you've considered
If there's a reason why it could be desired to have the default value from the overloaded function shown, this could be limited to cases where the ellipsis is used as the default value for the overloaded function. I don't think there would be such a reason, but I might be missing something due to how I use these.

Additional context
None

@Jackenmen Jackenmen added the type:enhancement enhance or introduce a new feature label Dec 2, 2020
@tk0miya
Copy link
Member

tk0miya commented Dec 12, 2020

(a common idiom when using @typing.overload):

I've not known such an idiom. Could you let me know a pointer about it?

@cleoold
Copy link

cleoold commented Jan 14, 2021

(a common idiom when using @typing.overload):

I've not known such an idiom. Could you let me know a pointer about it?

Here are some descriptions on mypy website: https://mypy.readthedocs.io/en/stable/stubs.html?highlight=overload#using-stub-file-syntax-at-runtime

Meanwhile in pyright (vs code) library stubs are defined this way.

@Jackenmen
Copy link
Author

Wow, looks like this got lost in my read email, sorry for that. What cleoold said is basically what I meant. There's one more thing that I want to present with a bit better code example from what I presented in the issue description.

So, aside from this somewhat being an idiom, some method signatures actually need to do this to avoid runtime effects (either some actual side effect or just some action taking a long time).

Here's an example of what I mean by that (the signature is probably very untypical, but it should be a good enough example presenting the performance cost of defining overload with a default):

@overload
def foo(bar: str) -> int:
    ...

@overload
def foo(bar: Pattern[str] = re.compile("default pattern")) -> str:
    ...

def foo(bar: Union[str, Pattern[str]] = re.compile("default pattern")) -> str:
    """Some doc"""
    # actual implementation

Note: Strictly speaking, re.compile caches patterns so performance cost wouldn't actually be much, but if it didn't cache patterns, it would be more significant


Oh, and one issue I noticed with the code I put in the issue description. It doesn't matter much from Sphinx's perspective I guess, but I still want to correct it. The variant with str passed should actually not have a default specified:

    @overload
    async def get_shared_api_tokens(self, service_name: str) -> Dict[str, str]:
        ...
    # everything else as in the issue description

@tk0miya
Copy link
Member

tk0miya commented Jan 16, 2021

Thank you for the pointer. I don't know the technique. Okay, let's adopt the idiom.

@tk0miya tk0miya added this to the 3.5.0 milestone Jan 16, 2021
tk0miya added a commit to tk0miya/sphinx that referenced this issue Jan 16, 2021
… from actual implementation

As a well-known idiom, mypy recommends to use ellipsis ("...") for
default argument values as a elided style.  This allows to write the
style and helps to document it with copying the default argument
values from actual implementation.

Note: This does not copy the default argument value when the argument
of overloaded function has its own default value.
tk0miya added a commit to tk0miya/sphinx that referenced this issue Jan 16, 2021
… from actual implementation

As a well-known idiom, mypy recommends to use ellipsis ("...") for
default argument values as a elided style.  This allows to write the
style and helps to document it with copying the default argument
values from actual implementation.

Note: This does not copy the default argument value when the argument
of overloaded function has its own default value.
tk0miya added a commit that referenced this issue Jan 17, 2021
Close #8514: autodoc: Default values of overloads are taken from actual implementation
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions:autodoc type:enhancement enhance or introduce a new feature
Projects
None yet
Development

No branches or pull requests

3 participants