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

Add typing to BaseReporter.out #5023

Merged
merged 5 commits into from
Sep 17, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Sep 16, 2021

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
😨 Refactor

Description

Small changes which are needed for further typing of pylint/reporters.
I ran the test suite and printed out all types of BaseReporter.out. It was only ever io.TextIO which also seems fair given what it is doing. However, perhaps somebody with more knowledge of the code base wants to take a look at this.

@@ -122,11 +121,11 @@ def _display(self, layout: "Section") -> None:
pass

@property
def out(self) -> StringIO:
def out(self) -> TextIO: # type: ignore # Property is not the same as BaseLinter's attr
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cdce8p You also commented on this in #4954, see #4954 (comment) and #4954 (comment)
However, this still is an issue. mypy maintainers seem to suggest not doing this kind of stuff (see python/mypy#8185). Do we want to refactor (can we?) or is adding the type: ignore enough. For some reason for the one below I needed two comments.

Copy link
Member

Choose a reason for hiding this comment

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

I've come across python/mypy#8185 in another project already. Was doing something similar 😅 type: ignore would be fine IMO. See also python/mypy#7994 which suggest to use type: ignore to silence the error.

Suggested change
def out(self) -> TextIO: # type: ignore # Property is not the same as BaseLinter's attr
def out(self) -> TextIO: # type: ignore[override]

@coveralls
Copy link

coveralls commented Sep 16, 2021

Pull Request Test Coverage Report for Build 1244657760

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.0005%) to 93.055%

Files with Coverage Reduction New Missed Lines %
pylint/reporters/reports_handler_mix_in.py 2 95.56%
Totals Coverage Status
Change from base Build 1243166105: 0.0005%
Covered Lines: 13319
Relevant Lines: 14313

💛 - Coveralls

Comment on lines 127 to 128
@property # type: ignore # Property is not the same as BaseLinter's attr
def linter(self) -> PyLinter: # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

type: ignore[override] should only be necessary on the second line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, but mypy doesn't. Note that we also don't get a "useless suppression" warning. Might want to report this as a bug to mypy, but both comments are necessary for mypy to pass.

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 17, 2021
pylint/reporters/base_reporter.py Outdated Show resolved Hide resolved
pylint/reporters/base_reporter.py Outdated Show resolved Hide resolved
pylint/reporters/base_reporter.py Outdated Show resolved Hide resolved
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
pylint/reporters/base_reporter.py Outdated Show resolved Hide resolved
pylint/reporters/base_reporter.py Outdated Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

Turns out some reporters are initialised while specifically supplying None

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.11.2 milestone Sep 17, 2021
@Pierre-Sassoulas Pierre-Sassoulas merged commit 096b31f into pylint-dev:main Sep 17, 2021
@DanielNoord DanielNoord deleted the typing-basereporter branch September 17, 2021 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants