Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test: bump pyright to 1.1.377 #1332

Merged
merged 4 commits into from
Aug 28, 2024
Merged

test: bump pyright to 1.1.377 #1332

merged 4 commits into from
Aug 28, 2024

Conversation

dimaqq
Copy link
Contributor

@dimaqq dimaqq commented Aug 23, 2024

In preparation to enabling reportUnnecessaryTypeIgnoreComment, bumping pyright to today's latest.

super-tox run: OK (tested on host, macos)

  • 58 charms pass against canoincal/operator@main
  • 58 charms pass against dimaqq/operator@bump-pyright

Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

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

Is the switch to the future import required for the pyright change? I'm happy to make that change, but if it's not part of the required changes could you add some info about it to the PR description?

test/test_main.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

Can we also make sure that charms that use MyPy still type check okay? I did a fair bit of work when updating the types to ensure that charms could use either Pyright or MyPy. We should probably have a some kind of automated test for this.

@dimaqq dimaqq merged commit 2facdea into canonical:main Aug 28, 2024
34 checks passed
@dimaqq dimaqq deleted the bump-pyright branch August 28, 2024 03:57
dimaqq added a commit that referenced this pull request Aug 28, 2024
Parent: https://warthogs.atlassian.net/browse/CHARMTECH-232

Covered in the parent epic:

- charmcraft analyse will validate that charm Python entrypoint calls
ops.main
- it is static type checker’s job to ensure correct arguments to
ops.main
- today, many charms do and have to do ops.main(ThisCharm) # type:
ignore
- thus, type hints need to be improved so that typpe: ignore can be
dropped

 

Plan in this story:

- upgrade pyright (not sure if strictly necessary, #1332)
- enabled pyright setting reportUnnecessaryTypeIgnoreComment
- include code that calls ops.main(SomeCharm) in every which legal way
in ops tests somewhere
- pyright validates the positive case, that ops.main can be called with
a charm class
- include code that calls ops.main() without a charm class
   - pyright now validates that such invocation is detected as an error
   - thus, this invocation is annotated with # type: ignore or similar
- with the setting above, pyright also enforce that the annotation is
actually needed


Ref: "How to write negative type tests?"
microsoft/pyright#8803

Part 2, based on #1332 


MyPy checks, pinned mypy:
- [x] `nginx-ingress-integrator-operator` mypy 1.6.1 OK
- [x] `sdcore-webui-operator` mypy 1.11.1 OK

Super-tox, running on host (macos)
- 58 charms pass against canonical/operator@main
- 58 charms pass against
dimaqq/aoperator@pyright-reportUnnecessaryTypeIgnoreComment
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants