Skip to content

Stricter types with pyright #2731

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 18 commits into from
Aug 10, 2025
Merged

Stricter types with pyright #2731

merged 18 commits into from
Aug 10, 2025

Conversation

alexrudd2
Copy link
Collaborator

@alexrudd2 alexrudd2 commented Aug 9, 2025

pyright is another type-checker that finds some things that mypy does not.

Currently there are 42 errors in dev, which this PR reduces to 6. EDIT: 32.

Several of them are false positives from imports and byte/bytearray confusion.
However, many are mismatched type signatures between classes and subclasses that are easy enough to improve.

I have split different types of changes into different commits so they can be cherry-picked or reverted if desired.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

Not bad, but I think you did a bit too much search/replace, especially the _x parameters give important information.

@alexrudd2
Copy link
Collaborator Author

I fully expected some pushback on the _unused_arg / unused_arg so they're in separate commits.

Alternatives:

class Base:
    def method(arg):
        return arg

# this PR - keep identical signatures
class Derived:
    def method(arg):
        pass

# explicit suppress
class Derived_ignore:   # type:ignore[reportIncompatibleMethodOverride]   
    def method(_arg):   
        pass

# fake usage
class Derived_assign:
    def method(arg):
        _ = arg

# unpacking
class Derived_kwargs:
   def method(*args, **kwargs):
      pass

@janiversen
Copy link
Collaborator

But this is wrong !! it does not make sense to demand that positional parameters have the same name in inherited classes....I would consider that a general false positive.

@alexrudd2
Copy link
Collaborator Author

But this is wrong !! it does not make sense to demand that positional parameters have the same name in inherited classes....I would consider that a general false positive.

Disagree, although it's not a particularly big deal if using positional args: https://en.wikipedia.org/wiki/Liskov_substitution_principle

@alexrudd2
Copy link
Collaborator Author

Anyways, I have removed the contentious commits for another day. Do you agree with c3ed1b1 or should I revert that as well?
#2731 (comment)

@janiversen
Copy link
Collaborator

I think you made that commit a separate PR, which I have approved.

@alexrudd2
Copy link
Collaborator Author

I think you made that commit a separate PR, which I have approved.

Yes, if you can't solve a problem... remove it 😆

Anyways, I think this PR I ready.

@janiversen
Copy link
Collaborator

Let me take a look.

Copy link
Collaborator

@janiversen janiversen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@janiversen janiversen merged commit 36818f8 into dev Aug 10, 2025
18 checks passed
@janiversen janiversen deleted the pyright branch August 10, 2025 16:18
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