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 all calls to self.stats #4973

Merged
merged 10 commits into from Sep 15, 2021

Conversation

DanielNoord
Copy link
Member

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

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

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

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 PyCQA#4954
@coveralls
Copy link

coveralls commented Sep 6, 2021

Pull Request Test Coverage Report for Build 1235382760

  • 80 of 80 (100.0%) changed or added relevant lines in 16 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 93.089%

Totals Coverage Status
Change from base Build 1235242213: 0.01%
Covered Lines: 13282
Relevant Lines: 14268

πŸ’› - Coveralls

@@ -95,8 +95,14 @@ def on_set_current_module(self, module: str, filepath: Optional[AnyPath]) -> Non

def on_close(
self,
stats: Mapping[Any, Any],
previous_stats: Mapping[Any, Any],
stats: Dict[
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to Dict instead of Mapping as its only ever called with self.stats and using Mapping gave a Mypy warning because of an incorrect argument type for one of the calls to on_close.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I was thinking about creating a class so the typing is simpler and we can unit test it easily.

def table_lines_from_stats(stats, old_stats, columns):
def table_lines_from_stats(
stats: Dict[
str, Union[int, Counter[str], List, Dict[str, Union[int, str, Dict[str, int]]]]
Copy link
Member Author

Choose a reason for hiding this comment

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

The List included in the type of all stats comes from

self.linter.add_stats(cycles=[])

This is then used in

pylint/pylint/graph.py

Lines 170 to 182 in 40cc2ff

def get_cycles(graph_dict, vertices=None):
"""given a dictionary representing an ordered graph (i.e. key are vertices
and values is a list of destination vertices representing edges), return a
list of detected cycles
"""
if not graph_dict:
return ()
result = []
if vertices is None:
vertices = graph_dict.keys()
for vertice in vertices:
_get_cycles(graph_dict, [], set(), result, vertice)
return result

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Not a full review just yet

pylint/checkers/__init__.py Outdated Show resolved Hide resolved
pylint/checkers/__init__.py Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@
from unittest.mock import Mock

import pytest
from conftest import PyreverseConfig # type: ignore
from conftest import PyreverseConfig # type: ignore #pylint: disable=no-name-in-module
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason the import of conftest introduced in 2500086 failed the pre-commit.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be from pylint.pyrevers.conftest import PyreverseConfig?

Copy link
Member Author

Choose a reason for hiding this comment

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

See discussion here: #4950 (comment)
Seems like we can't fix this without a (big) refactor

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

I still feel like I need to think about this a bit longer. Left some comments nevertheless.

pylint/checkers/__init__.py Outdated Show resolved Hide resolved
pylint/checkers/base.py Outdated Show resolved Hide resolved
pylint/checkers/raw_metrics.py Outdated Show resolved Hide resolved
pylint/lint/parallel.py Outdated Show resolved Hide resolved
pylint/lint/parallel.py Outdated Show resolved Hide resolved
pylint/lint/parallel.py Outdated Show resolved Hide resolved
pylint/lint/report_functions.py Outdated Show resolved Hide resolved
pylint/typing.py Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@
from unittest.mock import Mock

import pytest
from conftest import PyreverseConfig # type: ignore
from conftest import PyreverseConfig # type: ignore #pylint: disable=no-name-in-module
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be from pylint.pyrevers.conftest import PyreverseConfig?

@DanielNoord DanielNoord mentioned this pull request Sep 7, 2021
2 tasks
@cdce8p cdce8p added this to the 2.12.0 milestone Sep 8, 2021
@DanielNoord
Copy link
Member Author

@Pierre-Sassoulas You might want to change the milestone on this (or other PRs). Same goes for #4980 in order for the current typing efforts of you and me not to become double-work.

Just to let you know, I didn't plan on opening any new typing PRs before the current ones are all merged. Probably best to work on getting the current ones merged first. Perhaps you could help me with a review of #4980 as it requires quite extensive knowledge of pylint πŸ˜„

pylint/lint/parallel.py Show resolved Hide resolved
pylint/reporters/reports_handler_mix_in.py Outdated Show resolved Hide resolved
pylint/reporters/ureports/nodes.py Outdated Show resolved Hide resolved
pylint/typing.py Outdated Show resolved Hide resolved
pylint/typing.py Outdated Show resolved Hide resolved
pylint/typing.py Show resolved Hide resolved
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.12.0, 2.11.0 Sep 14, 2021
@cdce8p cdce8p mentioned this pull request Sep 14, 2021
@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 14, 2021
pylint/checkers/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

@Pierre-Sassoulas Would you mind taking a look at this? Overall I think it's good. Haven't checked each type tough.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

I like the new class name, and this cleans up a lot of potential problem too πŸ‘

@Pierre-Sassoulas
Copy link
Member

Shouldn't this be from pylint.pyrevers.conftest import PyreverseConfig?

Yes, I opened #5010

@Pierre-Sassoulas Pierre-Sassoulas merged commit 22e56c0 into PyCQA:main Sep 15, 2021
31 checks passed
@DanielNoord DanielNoord deleted the typing-stats branch September 15, 2021 18:42
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.

None yet

4 participants