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

[Dev] Make the decorators type aware #4782

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Feb 20, 2025

Fixes #4353

Note: this PR is mainly AI generated. Need to be refined.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

@andylizf
Copy link
Collaborator

I attempted a similar change about 3 months ago. Adding type hints to decorators exposed numerous previously hidden type errors in the functions being decorated (which Pylint was ignoring before). This caused a cascade of new Pylint errors across the codebase, which is why I put it on hold at that time. Note that some of these type checks might require a newer version of Pylint to handle properly.

@Michaelvll
Copy link
Collaborator Author

I attempted a similar change about 3 months ago. Adding type hints to decorators exposed numerous previously hidden type errors in the functions being decorated (which Pylint was ignoring before). This caused a cascade of new Pylint errors across the codebase, which is why I put it on hold at that time. Note that some of these type checks might require a newer version of Pylint to handle properly.

Yea... we should fix this. Those additional errors could cause issues failed to be detected like #4770

@cg505
Copy link
Collaborator

cg505 commented Feb 24, 2025

I also tried this in like november haha. We should 100% do this and fix all the problems, though!

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.

decorated functions are not properly typechecked
3 participants