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

CI/TYP: enable reportGeneralTypeIssues for subset of files #47252

Merged
merged 12 commits into from
Jun 8, 2022
Merged

CI/TYP: enable reportGeneralTypeIssues for subset of files #47252

merged 12 commits into from
Jun 8, 2022

Conversation

twoertwein
Copy link
Member

Second attempt at enabling pyright's most important type checking rule (reportGeneralTypeIssues). Instead of having many file-based ignores as in #44855, this PR adds a second pyright configuration file just for reportGeneralTypeIssues and lists all excluded file in this one config file.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@twoertwein twoertwein marked this pull request as ready for review June 6, 2022 03:23
@@ -41,7 +43,7 @@ def _get_cells(self, left, right, vertical) -> tuple[int, int]:
hcells = sum([self._shape(df)[1] for df in left] + [self._shape(right)[1]])
return hcells, vcells

def plot(self, left, right, labels=None, vertical: bool = True):
def plot(self, left, right, labels: Sequence[str] = (), vertical: bool = True):
Copy link
Member

Choose a reason for hiding this comment

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

😱

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, it could even be Iterable[str] (only used within zip). And yes a string is okay for this function :)

Changed the default value as there is no check whether labels is None. Would raise if labels is not provided.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to Iterable[str] (still contains str and even scarier types)

@jreback jreback added the Typing type annotations, mypy/pyright type checking label Jun 6, 2022
@@ -26,7 +27,7 @@ from enum import Enum
class _NoDefault(Enum):
no_default = ...

no_default = _NoDefault.no_default
no_default: Final = _NoDefault.no_default
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed for pyright see microsoft/pyright#3546 (comment)

@twoertwein
Copy link
Member Author

If we encounter pyright errors in the future that are difficult to fix but not reported by mypy, we have a few options:

  1. add the entire file to the exclude list
  2. add a file-based comments # pyright: reportGeneralTypeIssues = false
  3. add a comment at the offending line # pyright: ignore or # pyright: ignore [reportGeneralTypeIssues]
  4. if it is a mistake in pyright: report it and there is probably a release within less than a week with the fix (the release cycle of pyright is amazingly fast)

As long as the exclude list is long, I would simply add the entire file to the exclude list. Once the exclude list becomes reasonably small, we could transition to using line-based comments (currently we would need to add ~650 line-based comments) and merge the two pyright configurations (pyproject.toml and pyright_reportGeneralTypeIssues.json).

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @twoertwein LGTM

this is fine (also how we did check_untyped_defs for mypy, start with a file based exclude list, then inline the excludes to turn on globally once had reduced considerably)

@mroeschke mroeschke added this to the 1.5 milestone Jun 8, 2022
@mroeschke mroeschke merged commit 5e3b51d into pandas-dev:main Jun 8, 2022
@mroeschke
Copy link
Member

Thanks @twoertwein. Would be great to create an issue to tackle the typing issues in the config file

@twoertwein twoertwein deleted the pyright_general branch June 8, 2022 19:26
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
…v#47252)

* CI/TYP: enable reportGeneralTypeIssues for subset of files

* labels can be Iterable[str]

* fix no_default

* specify pyright version only once

* TYP: remove mypy ignore from localization.py (pandas-dev#47240)

* TYP: remove mypy ignore from localization.py

* fixup! TYP: remove mypy ignore from localization.py

* specify pyright version only once

* bump pyright

* Update pyright_reportGeneralTypeIssues.json

Co-authored-by: Thierry Moisan <[email protected]>
Co-authored-by: Matthew Roeschke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants