Typing for `./pylint`, batch 1 #4954

DanielNoord · 2021-09-02T11:02:26Z

Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

Similar to #4950 but now for ./pylint, going directory for directory.
For this PR I would like to do 3 directories:

./pylint/utils
./pylint/testutils
./pylint/reporters

pylint/checkers/similar.py

pylint/utils/ast_walker.py

pylint/utils/utils.py

cdce8p · 2021-09-02T12:56:24Z

I would suggest to wait until after at least #4940 is merged.

DanielNoord · 2021-09-02T13:27:06Z

pylint/testutils/output_line.py

        if isinstance(other, Message):
            if self.confidence and other.confidence:
                return super().__eq__(other)
-            return self[:-1] == other[:-1]
+            return self[:-1] == other[:-1]  # type: ignore


I could not get this to work... Perhaps somebody else wants to take a look at this issue and how to solve it!

DanielNoord · 2021-09-02T15:36:54Z

Tests are failing on the fact that re.Pattern is not available in Python 3.6.
Is there a known workaround for this? On two places I use isinstance(x, re.Pattern). Can that be overwritten by something like isinstance(x, re.BaseClass) which is available in 3.6?

Pierre-Sassoulas · 2021-09-02T19:47:18Z

Is there a known workaround for this? On two places I use isinstance(x, re.Pattern). Can that be overwritten by something like isinstance(x, re.BaseClass) which is available in 3.6?

We could wait until the 23rd of December 2021 and drop python 3.6 before adding this typing and removing the type: ignore ? 😄 Might not be worth our time to fix this issue in 3.6 as very soon we won't have to.

DanielNoord · 2021-09-02T19:51:09Z

I'll do that when I get back to my computer. Need to wait for #4940 anyway.

coveralls · 2021-09-02T21:43:33Z

Pull Request Test Coverage Report for Build 1269333255

10 of 10 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 93.095%

Totals
Change from base Build 1267723310:	0.001%
Covered Lines:	13360
Relevant Lines:	14351

💛 - Coveralls

DanielNoord · 2021-09-02T21:52:19Z

Any idea why the docs build is failing? Should I add pytest to the dependencies?

cdce8p · 2021-09-02T21:56:38Z

Any idea why the docs build is failing? Should I add pytest to the dependencies?

Better to guard the pytest import behind if TYPE_CHECKING and to use strings for the imported types (from pytest).

pylint/checkers/similar.py

pylint/lint/pylinter.py

pylint/reporters/reports_handler_mix_in.py

pylint/reporters/text.py

pylint/testutils/output_line.py

pylint/checkers/base.py

DanielNoord · 2021-09-04T21:02:26Z

There are some type: ignore's in this final version. However, I would be in favour of merging this and adding typing to all other folders and then get back to those comments.
Some of those require extensive refactoring for which I currently do not have the time.

pylint/extensions/docparams.py

pylint/extensions/typing.py

cdce8p

Thanks for your work, but tbh this was really difficult to review. It would be better if you could limit typing changes to much smaller PRs in the future.

One thing that stood out to me was Union[BufferedReader, BytesIO, TextIO]. It might be better to use a common base class where possible. Something like IOBase. I annotated it a bunch of times, haven't checked it in details though.

--
One last comment. To make the review process a bit easier, I would appreciate if you don't resolve any of the conversations. I won't be able to find comments otherwise. If you agree, simply add a 👍🏻 and change the code, I'll later go through and resolve them. If you like to start a conversation on a resolved item, please unresolve it.

cdce8p · 2021-09-05T19:11:26Z

pylint/checkers/similar.py

+    def append_stream(
+        self,
+        streamid: str,
+        stream: Union[BufferedReader, BytesIO, TextIO],


Can we use IOBase here?

cdce8p · 2021-09-05T19:12:42Z

pylint/checkers/similar.py

        """append a file to search for similarities"""
        if encoding is None:
-            readlines = stream.readlines
+            readlines: Union[Callable[..., Union[str, bytes]], Any] = stream.readlines


Why add an annotation at all?

I thought adding typing would be better so we can start moving to adding more mypy flags such as --disallow-untyped-defs etc.
I can add a type: ignore if you want, but personally I like how Any typings serve as a visual reminder that something is left to be done and allow searching for them within the project directory to create a "typing to-do list".

pylint/extensions/code_style.py

pylint/extensions/docparams.py

cdce8p · 2021-09-05T20:18:17Z

pylint/utils/utils.py

+    stream: Union[BufferedReader, BytesIO, TextIO],
+    encoding: str,
+    errors: str = "strict",
+) -> Union[latin_1_StreamReader, utf_8_StreamReader, cp1252_StreamReader]:


Can't you use a base class here? codecs.StreamReader?

pylint/utils/utils.py

cdce8p · 2021-09-05T20:25:13Z

tests/test_self.py

@@ -120,11 +120,11 @@ def _display(self, layout: EvaluationSection) -> None:
        pass

    @property
-    def out(self) -> StringIO:
+    def out(self) -> Union[TextIO, StringIO, None]:  # type: ignore # mypy doesn't seem to recognize properties


Properties are usually not an issue for mypy.
IOBase?

cdce8p · 2021-09-05T20:25:27Z

tests/test_self.py

+    @property  # type: ignore # mypy doesn't seem to recognize properties
+    def linter(self) -> Optional[PyLinter]:  # type: ignore # See comments above


Same here with the type: ignore comments

cdce8p · 2021-09-05T20:25:52Z

tests/utils/unittest_ast_walker.py

@@ -42,7 +42,7 @@ def test_check_messages(self) -> None:
        linter = self.MockLinter(
            {"first-message": True, "second-message": False, "third-message": True}
        )
-        walker = ASTWalker(linter)
+        walker = ASTWalker(linter)  # type: ignore # Can't import MockLinter. This needs a refactor


The issue is that MockLinter doesn't extend PyLinter

Yes, but I also can't type MockLinter here. What do you want me to do?

DanielNoord · 2021-09-05T22:04:06Z

I'm truly sorry for making this PR too large. I got a bit carried away when working and forgot to keep a close eye on how truly large this PR had become. If you want I can still split it up based on the three directories and created individual PR's based on your feedback?

I'm working on incorporating all of your feedback. One of the issues I ran into is that TextIO doesn't inherit from IOBase. It only inherits from IO. That class is often too broad for typing and would lead to additional isinstance checks to satisfy mypy. TextIO is returned by some of the functions in the sys library and is therefore also needed as a recurring type. Previously I solved this by adding the Union's. I can see why you don't like that solution, but I'm stuck thinking of something better.
TLDR: IO creates mypy issues because of its broadness and the lack of certain standard attributes and methods of its sub-classes, TextIO doesn't inherit from IOBase but is returned by sys functions, current Union's are too broad and ugly.

I'm sorry for resolving the conversation prematurely. I was used to resolving after incorporating feedback but I can see how this is unhelpful for you. I appreciate the effort you took to still review this PR instead of dismissing it because of its size (which I have seen happen in other projects). Thank you!

cdce8p · 2021-09-05T23:01:51Z

I haven't taken a look at all the review comments yet. Only want to answer some of your questions now.

If you want I can still split it up based on the three directories and created individual PR's based on your feedback?

In general smaller PRs might be better. Don't know though if directory bases is the best approach. Although I know it will be more work, I would think of it in related topics. Just to name some examples:

Everything dealing with self.stats
stream and IOBase
get_global_option and related type: ignore comments. (I would appreciate this one especially!)
filepath
... (there might be more, but that's a start)

One of the issues I ran into is that TextIO doesn't inherit from IOBase.

To be honest with you, I haven't checked it in detail if it would work / where it might not. (There were just too many changes.)
Separating those into a new PR might be a way to better understand the relationships between all of them. In the end, if Union is the only thing that works, so be it.

I'm sorry for resolving the conversation prematurely. I was used to resolving after incorporating feedback but I can see how this is unhelpful for you.

It's nothing personal. With so many different conversations I just felt that otherwise I couldn't follow it properly.

I appreciate the effort you took to still review this PR instead of dismissing it because of its size (which I have seen happen in other projects). Thank you!

I can understand why it sometimes happens. Most maintainers, I would guess, don't know every inch of their code of the top of their head. So it takes a lot of mental energy to go through it if at all.

Pierre-Sassoulas

Thanks ! This uncovered so many things that ideally we'd have to refactor, it's definitely a step in the right direction.

All checkers inherit from a baseclass which has a ``stats`` attribute. This attribute has a fairly unmanagable type, but the current typing includes all variations of the attribute. Other changes not directly related to ``self.stats`` are due to ``mypy`` warnings. This incorporate the feedback received in pylint-dev#4954

All checkers inherit from a baseclass which has a ``stats`` attribute. This attribute has a fairly unmanageable type, but the current typing includes all variations of the attribute. Other changes not directly related to ``self.stats`` are due to ``mypy``warnings. This incorporate the feedback received in pylint-dev#4954

* Add typing to all calls to ``self.stats`` All checkers inherit from a baseclass which has a ``stats`` attribute. This attribute has a fairly unmanageable type, but the current typing includes all variations of the attribute. Other changes not directly related to ``self.stats`` are due to ``mypy``warnings. This incorporate the feedback received in #4954 * Add ``CheckerStatistic`` class to ``pylint/typing`` * Guard `typing.Counter` import Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Pierre-Sassoulas

👍

DanielNoord commented Sep 2, 2021

View reviewed changes

pylint/checkers/similar.py Outdated Show resolved Hide resolved

DanielNoord commented Sep 2, 2021

View reviewed changes

pylint/utils/ast_walker.py Outdated Show resolved Hide resolved

DanielNoord commented Sep 2, 2021

View reviewed changes

pylint/utils/utils.py Outdated Show resolved Hide resolved

DanielNoord force-pushed the pyannotate-pylint branch from e365ef6 to 4bcff3e Compare September 2, 2021 12:53

DanielNoord commented Sep 2, 2021

View reviewed changes

DanielNoord marked this pull request as ready for review September 2, 2021 15:35

Pierre-Sassoulas added this to the 2.11.x milestone Sep 2, 2021

Pierre-Sassoulas reviewed Sep 3, 2021

View reviewed changes

cdce8p self-requested a review September 3, 2021 15:33

Pierre-Sassoulas requested changes Sep 4, 2021

View reviewed changes

pylint/checkers/base.py Outdated Show resolved Hide resolved

DanielNoord commented Sep 5, 2021

View reviewed changes

pylint/extensions/docparams.py Outdated Show resolved Hide resolved

DanielNoord commented Sep 5, 2021

View reviewed changes

pylint/extensions/typing.py Outdated Show resolved Hide resolved

Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 5, 2021

Pierre-Sassoulas self-requested a review September 5, 2021 09:01

cdce8p requested changes Sep 5, 2021

View reviewed changes

DanielNoord marked this pull request as draft September 5, 2021 22:13

Pierre-Sassoulas approved these changes Sep 6, 2021

View reviewed changes

This was referenced Sep 6, 2021

Add typing to all calls to self.stats #4973

Merged

Use overload to type get_global_option #4977

Closed

Use overload to type get_global_option #4978

Merged

Add typing to references of stream and stream() #4983

Merged

DanielNoord force-pushed the pyannotate-pylint branch from 8a4ac63 to e5a9146 Compare September 16, 2021 19:38

This was referenced Sep 16, 2021

Add typing for json_reporter and sub-classes #5021

Merged

Add typing to BaseReporter.out #5023

Merged

Fix typing of ReportsHandlerMixIn #5028

Merged

DanielNoord force-pushed the pyannotate-pylint branch from f9b4ee0 to d8bc481 Compare September 17, 2021 08:09

This was referenced Sep 17, 2021

Add typing to lint_module_test #5035

Merged

Add typing to text reporter #5041

Merged

DanielNoord force-pushed the pyannotate-pylint branch 3 times, most recently from b562f3e to 6c8f111 Compare September 24, 2021 09:02

Add small bits of typing annotation

6309d21

DanielNoord force-pushed the pyannotate-pylint branch from 6c8f111 to 6309d21 Compare September 24, 2021 09:53

DanielNoord marked this pull request as ready for review September 24, 2021 10:08

DanielNoord requested a review from Pierre-Sassoulas September 24, 2021 10:08

Pierre-Sassoulas approved these changes Sep 24, 2021

View reviewed changes

Pierre-Sassoulas requested a review from cdce8p September 24, 2021 15:58

cdce8p approved these changes Sep 24, 2021

View reviewed changes

Pierre-Sassoulas merged commit ae4e39e into pylint-dev:main Sep 25, 2021

DanielNoord deleted the pyannotate-pylint branch September 26, 2021 10:41

cdce8p modified the milestones: 2.13.x, 2.12.0 Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing for `./pylint`, batch 1 #4954

Typing for `./pylint`, batch 1 #4954

DanielNoord commented Sep 2, 2021 •

edited

Loading

cdce8p commented Sep 2, 2021

DanielNoord Sep 2, 2021

DanielNoord commented Sep 2, 2021

Pierre-Sassoulas commented Sep 2, 2021

DanielNoord commented Sep 2, 2021

coveralls commented Sep 2, 2021 •

edited

Loading

DanielNoord commented Sep 2, 2021

cdce8p commented Sep 2, 2021

DanielNoord commented Sep 4, 2021

cdce8p left a comment

cdce8p Sep 5, 2021

cdce8p Sep 5, 2021

DanielNoord Sep 5, 2021

cdce8p Sep 5, 2021

cdce8p Sep 5, 2021

cdce8p Sep 5, 2021

cdce8p Sep 5, 2021

DanielNoord Sep 5, 2021

DanielNoord commented Sep 5, 2021

cdce8p commented Sep 5, 2021

Pierre-Sassoulas left a comment

Pierre-Sassoulas left a comment

		@property # type: ignore # mypy doesn't seem to recognize properties
		def linter(self) -> Optional[PyLinter]: # type: ignore # See comments above

Typing for ./pylint, batch 1 #4954

Typing for ./pylint, batch 1 #4954

Conversation

DanielNoord commented Sep 2, 2021 • edited Loading

Type of Changes

Description

cdce8p commented Sep 2, 2021

Choose a reason for hiding this comment

DanielNoord commented Sep 2, 2021

Pierre-Sassoulas commented Sep 2, 2021

DanielNoord commented Sep 2, 2021

coveralls commented Sep 2, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1269333255

💛 - Coveralls

DanielNoord commented Sep 2, 2021

cdce8p commented Sep 2, 2021

DanielNoord commented Sep 4, 2021

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Sep 5, 2021

cdce8p commented Sep 5, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Typing for `./pylint`, batch 1 #4954

Typing for `./pylint`, batch 1 #4954

DanielNoord commented Sep 2, 2021 •

edited

Loading

coveralls commented Sep 2, 2021 •

edited

Loading