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

Changing mypy to pyright #4425

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

pierceroberts
Copy link

@pierceroberts pierceroberts commented Feb 13, 2025

Description

This change can be documented here:
#4404

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Run pre-commit with out issues
  • python -m pyright runs successfully
  • python -m mypy does not run.

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Feb 13, 2025

CLA Missing ID CLA Not Signed

@pierceroberts
Copy link
Author

@emdneto and @Kludex for visibility. Still a few things to iron out. Feel free to comment as I go.

@@ -60,6 +60,11 @@ You can run `tox` with the following arguments:
- `tox -e lint-some-package` to run lint checks on `some-package`
- `tox -e generate-workflows` to run creation of new CI workflows if tox environments have been updated
- `tox -e ruff` to run ruff linter and formatter checks against the entire codebase
- `tox -e pyright` to run pyright against entire code base.
Copy link
Author

Choose a reason for hiding this comment

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

There were a few targets that were mentioned so I added them.

mypy-relaxed.ini Outdated
Copy link
Author

Choose a reason for hiding this comment

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

deleted these because the plan is to move away from mypy

Copy link
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

You need to get CLA signed to get things merged, so please take a look at the CLA error

]
reportUnnecessaryTypeIgnoreComment = true
pythonVersion = "3.9"
reportPrivateUsage = false # Ignore private attributes added by instrumentation packages.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate. In terms of typing, what opentelemetry does is wrong... It would be more useful to have a concrete example to discuss here.

"exporter/**",
"propagator/**",
"shim/**",

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert changes here, please.

Copy link
Author

Choose a reason for hiding this comment

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

Can I ask why we wouldn't want to use this version?

Copy link
Author

Choose a reason for hiding this comment

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

Oh I see what you mean, Delete this file and add it to dev requirements, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just add this to the dev-requirements? 🤔

@pierceroberts pierceroberts marked this pull request as ready for review February 19, 2025 17:09
@pierceroberts pierceroberts requested a review from a team as a code owner February 19, 2025 17:09
"shim/**",

]
reportUnnecessaryTypeIgnoreComment = true
Copy link
Author

@pierceroberts pierceroberts Feb 20, 2025

Choose a reason for hiding this comment

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

We should actually talk about all the config settings that aren't set for strict:
https://github.com/microsoft/pyright/blob/main/docs/configuration.md#:~:text=%22error%22-,reportCallInDefaultInitializer,-%22none%22
Maybe there are some that would be useful,
There are 10 of them.

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.

3 participants