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

Require explicit override decorator #1255

Open
wants to merge 10 commits into
base: 3.x
Choose a base branch
from

Conversation

bauen1
Copy link
Contributor

@bauen1 bauen1 commented Dec 19, 2024

Not sure if this is a good idea, hence a draft MR, there would be some 304 missing @override decorators.

@Fizzadar
Copy link
Member

Fizzadar commented Jan 3, 2025

I think this is a good change, an explicit typing improvement with no actual functionality change. Only concern might be breaking type-checking with other modules but that doesn't fall under the version compatibility anyway so probably OK...

@bauen1 bauen1 force-pushed the fix/explicit-override branch 2 times, most recently from 24740a3 to 75254cb Compare January 4, 2025 14:41
@bauen1 bauen1 force-pushed the fix/explicit-override branch from 75254cb to b9fa3a9 Compare January 9, 2025 15:57
@bauen1 bauen1 changed the title Draft: Require explicit override decorator Require explicit override decorator Jan 9, 2025
@bauen1
Copy link
Contributor Author

bauen1 commented Jan 9, 2025

Well, it passes the pipeline now, but locally I still get some errors:

jh@pns-admin:~/dev/pyinfra$ mypy
Warning: Unpack is already enabled by default
pyinfra/context.py:83: error: Incompatible types in assignment (expression has type "type[local]", base class "ContextObject" defined the type as "type[container]")  [assignment]
pyinfra_cli/__main__.py:34: error: Missing positional argument "greenlet" in call to "signal"  [call-arg]
pyinfra_cli/__main__.py:37: error: Module has no attribute "signal"  [attr-defined]
Found 3 errors in 2 files (checked 145 source files)

So I'm guessing there's some different version of greenlet installed in my container ?

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.

2 participants