Skip to content

Add peewee playhouse.flask_utils stubs #11731

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

Merged
merged 27 commits into from
Apr 23, 2024
Merged

Add peewee playhouse.flask_utils stubs #11731

merged 27 commits into from
Apr 23, 2024

Conversation

pylipp
Copy link
Contributor

@pylipp pylipp commented Apr 8, 2024

@AlexWaygood @mikeziminio seems like you have worked with peewee in the past, mind reviewing?

Source: https://github.com/coleifer/peewee/blob/master/playhouse/flask_utils.py

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@Avasam
Copy link
Collaborator

Avasam commented Apr 19, 2024

Would it be better to maybe add playhouse\.\w+? to stubs/peewee/@tests/stubtest_allowlist.txt to ignore missing modules in playhouse rather than ignore_missing_stub = true in METADATA.toml ? (which won't reveal missing new names added in updates like #11373 ).

I'd keep partial_stub = true though, with a comment explaining that we don't provide all playhouse modules (one of the rare cases where you'd have partial_stub w/o ignore_missing_stub)

This comment has been minimized.

This comment has been minimized.

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested a few improvements. Otherwise these changes look good.

pylipp and others added 7 commits April 19, 2024 18:05
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
Co-authored-by: Avasam <samuel.06@hotmail.com>
@Avasam Avasam changed the title Add peewee playhouse stubs Add peewee playhouse.flask_utils stubs Apr 19, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@Avasam
Copy link
Collaborator

Avasam commented Apr 20, 2024

I really get the feeling this is a stubtest false-positive because of the property Model.
It looks like stubtest is seeing model_class=Model and thinks its assigning the value of the Model property (as if it was called, likely because mypy special cases properties) rather than the Model class.

@hauntsaninja thoughts?

@Avasam
Copy link
Collaborator

Avasam commented Apr 22, 2024

@pylipp In the meantime, I think you can simply avoid typing the model_class param in __init__ to get your PR though.
It would have been nice to get the class fully typed from the get-go, but our peewee stubs are marked as incomplete anyway.

@pylipp
Copy link
Contributor Author

pylipp commented Apr 23, 2024

@Avasam thanks for sticking with this PR :) I'm learning a lot about writing stubs from your helpful explanations, really grateful for that and the welcoming culture.
I tried your latest suggestion in 0f2bc77, let's see how it goes

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Avasam
Copy link
Collaborator

Avasam commented Apr 23, 2024

Thanks!

@Avasam Avasam merged commit ed7f35a into python:main Apr 23, 2024
42 checks passed
@pylipp
Copy link
Contributor Author

pylipp commented Apr 25, 2024

@Avasam I was hoping to fix a name-defined error I saw in my code base but it still persists.
I added an MWE here. Do you mind taking a look?
I suspect it could be because of the "name clash" with Model you mentioned above.

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 this pull request may close these issues.

4 participants