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.

Sign up for GitHub

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

Jump to bottom

Add some manual typing changes #6325

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Add some manual typing changes #6325

merged 2 commits into from
Apr 14, 2022

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Apr 14, 2022 •
edited
Loading

  • 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

Ref #4683.

@DanielNoord DanielNoord requested a review from cdce8p April 14, 2022 18:09
@DanielNoord DanielNoord added this to the 2.14.0 milestone Apr 14, 2022
@DanielNoord DanielNoord added typing Maintenance Discussion or action around maintaining pylint or the dev workflow labels Apr 14, 2022
@coveralls
Copy link

coveralls commented Apr 14, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2169486554

  • 24 of 24 (100.0%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 93.376%
Totals Coverage Status
Change from base Build 2168825760: 0.003%
Covered Lines: 15972
Relevant Lines: 17105
💛 - Coveralls

cdce8p
cdce8p reviewed Apr 14, 2022
View reviewed changes
pylint/checkers/variables.py Outdated
@@ -516,7 +516,7 @@ class ScopeConsumer(NamedTuple):

to_consume: dict[str, list[nodes.NodeNG]]
consumed: dict[str, list[nodes.NodeNG]]
consumed_uncertain: DefaultDict[str, list[nodes.NodeNG]]
consumed_uncertain: collections.defaultdict[str, list[nodes.NodeNG]]
Copy link
Member

Choose a reason for hiding this comment

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

We can use defaultdict directly. Even if we already import collections.

from collections import defaultdict

@cdce8p cdce8p mentioned this pull request Apr 14, 2022
Merged
2 tasks
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @cdce8p that collections.DefaultDict is a little too verbose, otherwise LGTM.

@cdce8p cdce8p merged commit 2e0a4e7 into pylint-dev:main Apr 14, 2022
@DanielNoord
Copy link
Collaborator Author

Thanks @cdce8p for fixing and merging this. I was away this evening!

@DanielNoord DanielNoord deleted the 3.7-2 branch April 14, 2022 21:44
@cdce8p
Copy link
Member

cdce8p commented Apr 14, 2022

Thanks @cdce8p for fixing and merging this. I was away this evening!

No problem! I had checked out the branch locally anyway, so it wasn't a big deal.

@DanielNoord
Copy link
Collaborator Author

And thanks for the many reviews today! This could have taken a couple of days of back-and-forth if you hadn't been so active today! Much appreciated 😄

@cdce8p
Copy link
Member

cdce8p commented Apr 14, 2022

And thanks for the many reviews today! This could have taken a couple of days of back-and-forth if you hadn't been so active today! Much appreciated 😄

It's you who did the majority of the work while also fixing a regression! Thank you 🚀

Now we just need to do that again for astroid 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdce8p cdce8p cdce8p approved these changes

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow typing
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.
4 participants
@DanielNoord @coveralls @cdce8p @Pierre-Sassoulas