Skip to content

Add pyright for static type checking #17744

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 8 commits into from
Mar 6, 2025
Merged

Add pyright for static type checking #17744

merged 8 commits into from
Mar 6, 2025

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Feb 26, 2025

Link to issue number:

Related to #17301

Summary of the issue:

NVDA has no type checking for python.
Python is a dynamically typed language meaning typing issues can occur at runtime.
Static analysers like mypy or pyright can be used to avoid typing errors.

Description of user facing changes

None

Description of development approach

pyright was chosen over mypy for performance reasons.
Additionally, pyright has better pre-commit integration, and is built-in to VS Code, which is used by most NVDA developers.

We are far from compliant with static type checking, so most rules are currently disabled.
The intention is to fix compliance and turn on rules over time.

Various trivial typing fixes have occurred, particularly:

  • removing unnecessary casts
  • fixing type hints
  • adding relevant type ignore comments
  • removing redundant type ignore comments
  • adding type stubs for gettext builtins and _buildVersion generated build vars
  • remove duplicate imports
  • adding __all__ to mark export for private classes
  • adding missing match cases

Testing strategy:

  • Smoke test NVDA
  • pyright passing
  • Automated testing passes

Known issues with pull request:

We are far from compliant with static type checking, so most rules are currently disabled.
The intention is to fix compliance and turn on rules over time.

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@seanbudd seanbudd requested a review from a team as a code owner February 26, 2025 03:21
@seanbudd seanbudd added api-breaking-change release/blocking this issue blocks the milestone release labels Feb 26, 2025
@seanbudd seanbudd added this to the 2025.1 milestone Feb 26, 2025
@seanbudd seanbudd requested a review from a team as a code owner February 26, 2025 04:55
@hwf1324
Copy link
Contributor

hwf1324 commented Feb 26, 2025

Can you clean up the other trailing spaces in requirements.txt by the way?

@seanbudd
Copy link
Member Author

@hwf1324 - this is probably better done as its own PR, it's relatively trivial

@seanbudd
Copy link
Member Author

Note: there's no inbuilt way of automatically generating type: ignore comments like we can with ruff and noqa comments.
A custom script could be written to take the JSON output of pyright, and add comments using the data.
It would be great to do this and open source it for others

Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Mostly looks good, however I'm a little concerned about the inclusion of a bunch of internal classes in various modules' __all__.

@AppVeyorBot
Copy link

See test results for failed build of commit f2649a8e74

@seanbudd seanbudd requested a review from SaschaCowley March 4, 2025 01:24
@AppVeyorBot
Copy link

See test results for failed build of commit 445074b0b8

@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Mar 4, 2025
@AppVeyorBot
Copy link

See test results for failed build of commit defc94e191

Copy link
Member

@SaschaCowley SaschaCowley left a comment

Choose a reason for hiding this comment

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

Thanks @seanbudd

@SaschaCowley SaschaCowley merged commit f3c7d28 into master Mar 6, 2025
6 checks passed
@SaschaCowley SaschaCowley deleted the pyright branch March 6, 2025 23:38
seanbudd pushed a commit that referenced this pull request Mar 26, 2025
Follow-up to 17744

Summary of the issue:
As pointed out in #17744 (comment), the type hint for the dict argument to baseObject.ScriptableType.__new__ is incorrectly [str, Any] since #17744 was merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. release/blocking this issue blocks the milestone release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants