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

feat: Generate __init__ methods for typed dicts with **kwargs: Unpack[MyDict] to help docstring parser fetch other parameters types #284

Open
butterlyn opened this issue Jun 3, 2024 · 7 comments
Assignees
Labels
feature New feature or request fund Issue priority can be boosted

Comments

@butterlyn
Copy link

butterlyn commented Jun 3, 2024

Description of the bug

TypedDict classes gets a griffe warning "Parameter does not appear in the function signature" when correctly including the parameters in the docstring. (Note: TypedDict's do not include an explicit def __init__() constructor)

To Reproduce

class MyTypedDict(TypedDict):
    """
    Parameters
    ----------
    first_dict_key: str
        First dictionary key.
    second_dict_key: str
        Second dictionary key.
    """
    first_dict_key: str
    second_dict_key: str

Full traceback

$ mkdocs build --strict
WARNING -  griffe: src\aemo_databricks\sql\types.py:28: Parameter 'first_dict_key' does not appear in the function signature
WARNING -  griffe: src\aemo_databricks\sql\types.py:28: Parameter 'second_dict_key' does not appear in the function signature

Expected behavior

These warnings should not be raised, since (unlike normal python classes) first_dict_key and second_dict_key are parameters for instantiating the class, not class attributes.

Additional context

I'm using --strict flag for cicd testing of my MkDocs documentation, my cicd pipeline is failing due to this warning.

Boost priority

  • Boost priority in our backlog through Polar.sh. Higher pledge, higher priority.
  • Minimum pledge by user/organization is $5, minimum amount for boost is $30.
  • View all issues with pledges.
  • We receive the funds once the issue is completed and confirmed by you.
  • Features with the insiders label are released to sponsors first, and tied to a funding goal.
Fund with Polar
@butterlyn butterlyn added the unconfirmed This bug was not reproduced yet label Jun 3, 2024
@butterlyn butterlyn changed the title bug: bug: TypedDict docstring parameters raise warning Jun 3, 2024
@pawamoy
Copy link
Member

pawamoy commented Jun 4, 2024

Hi @butterlyn, thanks for reporting :)

Yes we should handle typed dicts like we handle data classes I suppose. I'll implement that asap.

@pawamoy
Copy link
Member

pawamoy commented Jun 7, 2024

Actually, they're not the same thing. Typed dicts will not create an __init__ method for you, like dataclasses do.

>>> from typing import TypedDict
>>> class D(TypedDict):
...     hello: str
...     world: int
... 
>>> D.__init__
<slot wrapper '__init__' of 'dict' objects>
>>> help(D.__init__)
Help on wrapper_descriptor:

__init__(self, /, *args, **kwargs)
    Initialize self.  See help(type(self)) for accurate signature.

>>> D(byebye=0.1)
{'byebye': 0.1}

If you want to force your users to use d = D(...) instead of d: D = {...: ...}, I recommend that you use a dataclass.

I do note that marking first_dict_key and second_dict_key as attributes of the class is a bit weird, API-ly speaking, but we should discuss this in another issue.

I will close as not planned.

@pawamoy pawamoy closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2024
@butterlyn
Copy link
Author

butterlyn commented Jun 8, 2024

Hi @pawamoy, thanks for the reply, though I'm a bit confused where to go from here.

I still need a way to add TypedDict items to a docstring in a way where griffe doesn't raise a warning. I've used parameters (not attributes) because:

  1. PEP 589 allows TypedDict to be called, where the TypedDict items act as parameters (quote below)
  2. The TypedDict items are not accessible with dot notation, so do not act as attributes

###
PEP 589

[TypedDict] can be used as a callable object with keyword arguments corresponding to the TypedDict items. Non-keyword arguments are not allowed. Example:

m = Movie(name='Blade Runner', year=1982)

When called, the TypedDict type object returns an ordinary dictionary object at runtime:

print(type(m))  # <class 'dict'>

###

Though I agree in almost all cases a dataclass (or pydantic model) is a better option, there are still cases where I need to use TypedDict for type hinting purposes and still need to include a docstring. I think parameters is the best class docstring heading that describes how TypedDict items behave in PEP 589 (though in an ideal world a new heading like "keys" would be supported). If you disagree, could you please advise how to add TypedDict docstrings in a way that's compatible with griffe?

@pawamoy
Copy link
Member

pawamoy commented Jun 8, 2024

Thanks for your patience.

The paragraphs from PEP 589 are just here for completeness I suppose: you get the same behavior with a basic dict, or a basic class that inherits from dict. The thing is, at runtime, you can pass any keyword, not just the ones you documented.

We're a bit between two worlds here, runtime and static analysis. I can see a way to reconcile them: if no __init__ method is defined in a class that inherits from TypedDict, Griffe could create one. Now, we have a few options for the __init__ method signature:

1. No lies: (/, *args, **kwargs)

That's the true signature of a typed dict.

PEP 589 is actually wrong (or unclear):

Non-keyword arguments are not allowed.

Look at me using positional arguments 🙂

>>> from typing import TypedDict
>>> class Movie(TypedDict):
...     name: str
...     year: int
... 
>>> Movie([("name", "Blade Runner"), ("year", 1982)])
{'name': 'Blade Runner', 'year': 1982}

I suppose what the PEP means by "not allowed" is "statically not allowed", i.e. type checkers will produce a warning. But at runtime it works fine.

2. Just kwargs: (**kwargs: Any)

class Movie(TypedDict):
    """A movie.

    Parameters:
        **kwargs: This docstring section is optional.

    Other parameters:
        name (str): The name of the movie.
        year (int): The year the movie was released.
    """

The issue, as you can see, is that you then have to duplicate the types in the docstring 🤔

3. Specific + kwargs: (*, movie: str, year: int, **kwargs)

We could consider making the signature more explicit, but that's not exactly correct, because movie and year would become required as they don't have default values. Well, with total=True or Required, that would be correct based on the developer intent, but still not correct at runtime.

class Movie(TypedDict):
    """A movie.

    Parameters:
        name: The name of the movie.
        year: The year the movie was released.
        **kwargs: This description is optional.
    """

4. Specific + None + kwargs: (*, movie: str | None = None, year: int | None = None, **kwargs)

But then the docs would show their type as allowing None, which is incorrect too.

5. Follow annotations

The idea would be to rebuild the signature based on type annotations (total=True/False, Required, NotRequired, etc.). That would require writing a complete extension I suppose, like for dataclasses.

6. Unpack?

We would create an __init__ method with this signature:

def __init__(self, **kwargs: Unpack[Movie]) -> None: ...

Griffe TypingDoc has some support for Unpack, we could add such support in Griffe too. With this you could document the parameters in an "Other parameters" section, without having to write the types again.


Alternative: give an "Items" title to the "Attributes" section

class Movie(TypedDict):
    """A movie.

    Attributes: Items:
        name: The name of the movie.
        year: The year the movie was released.
    """

However name and year will still be loaded as attributes, and mkdocstrings-python will still show an attr symbol type in front of them, etc.


So, what should we do here 🤷‍♂️? Griffe is not a type-checker, it tries to stay true to the runtime API. But at the same time, a library developer explicitly says to its users that this typed dict should be used in a specific way. We could consider that part of the API too. So, do we choose to lie? I worry that by lying about a signature, we enter too deep in the type-checking world. Example:

# version 1
class Movie(TypedDict):
    name: str
    year: int

Movie(name="a", year=0)  # OK
# version 2
class Movie(TypedDict):
    name: str
    year: int
    budget: int  # Griffe warning: parameter added as required (no default value)

Movie(name="a", year=0)  # but still OK at runtime... and ignore-able with a type: ignore comment

I don't like typed dicts anymore 🫠

WDYT ☺️?

@pawamoy
Copy link
Member

pawamoy commented Jun 8, 2024

Another alternative is to wait for #251 to be merged (but it's a big PR and still in draft, and it will probably end up in Insiders first). It will allow to disable some warnings (downgrade them to info or debug messages), but only globally, not per message.

@butterlyn
Copy link
Author

butterlyn commented Jun 9, 2024

Thanks @pawamoy for the detailed response, I can see how TypedDict makes this difficult 😅

Options 2 or 6 makes the most intuitive sense to me personally, "Other Parameters" seems like a nice way of characterising TypedDict items. Though this is merely my opinion which is subjective, I wouldn't want to cause more confusion by imposing my personal preferences. Happy to defer to you on which makes sense to implement (or wait for #251).

In the meantime, I'm annotating in the style of option 2 ("Other Parameters" and duplicating types in the docstring). This doesn't raise a griffe warning and resolves the cicd pipeline errors I was getting originally.

Thanks for your help with this issue 😄

@pawamoy
Copy link
Member

pawamoy commented Jun 9, 2024

Glad to see we agree: I also think option 2 and 6 are the best ones. Well, option 2 wouldn't be very useful since we can already use "Other parameters" and duplicated types, like you're doing. So ideally option 6 should be implemented, so that types can be fetched from the signature. Reopening.

@pawamoy pawamoy reopened this Jun 9, 2024
@pawamoy pawamoy changed the title bug: TypedDict docstring parameters raise warning feat: Generate __init__ methods for typed dicts with **kwargs: Unpack[MyDict] to help docstring parser fetch other parameters types Jun 9, 2024
@pawamoy pawamoy added feature New feature or request fund Issue priority can be boosted and removed unconfirmed This bug was not reproduced yet labels Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request fund Issue priority can be boosted
Projects
None yet
Development

No branches or pull requests

2 participants