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 regression test for #7506 #8432

Merged
merged 1 commit into from
Apr 3, 2023
Merged

Add regression test for #7506 #8432

merged 1 commit into from
Apr 3, 2023

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
✓ 🐛 Bug fix

Description

Closes #7506

Depends on pylint-dev/astroid#2049

@jacobtylerwalls jacobtylerwalls added Needs astroid update Needs an astroid update (probably a release too) before being mergable backport maintenance/3.3.x labels Mar 11, 2023
@jacobtylerwalls jacobtylerwalls added this to the 2.17.1 milestone Mar 11, 2023
jacobtylerwalls
jacobtylerwalls commented Mar 11, 2023
View reviewed changes
requirements_test.txt
@@ -7,5 +7,6 @@ pytest-cov~=4.0
pytest-profiling~=1.7
pytest-xdist~=3.2
# Type packages for mypy
six
Copy link
Member Author

Choose a reason for hiding this comment

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

We already had tests depending on six, and we were already installing six as a transitive dependency of pytest-profiling, but I figure it's better to make it explicit in case that ever changes.

jacobtylerwalls
jacobtylerwalls commented Mar 11, 2023
View reviewed changes
tests/functional/u/unused/unused_import.py
@@ -98,8 +99,10 @@ def blop(self):
import zoneinfo


class WithMetaclass(metaclass=ABCMeta):
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was altered in #7320 to remove six, but that made it unable to target the astroid behavior. The original test in #7153 didn't quite test this correctly: it confounded things by adding , object.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.17.1, 2.17.2 Mar 22, 2023
@jacobtylerwalls jacobtylerwalls removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Apr 1, 2023
@jacobtylerwalls jacobtylerwalls modified the milestones: 2.17.2, 3.0.0a6 Apr 1, 2023
@codecov
Copy link

codecov bot commented Apr 1, 2023

Codecov Report

Merging #8432 (591dddb) into main (e9894d3) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8432   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files         174      174           
  Lines       18357    18357           
=======================================
  Hits        17605    17605           
  Misses        752      752

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2023

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 591dddb

Pierre-Sassoulas
Pierre-Sassoulas reviewed Apr 1, 2023
View reviewed changes
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.

LGTM, but why couldn't it go in 2.17.2 ? There's going to be a lot to do before actually releasing 3.0.0 I think and we may support 2.X branch for some time.

@jacobtylerwalls
Copy link
Member Author

That would be ideal, but we would need to backport the PR that updated astroid to 2.15.1. Shall we do that?

@Pierre-Sassoulas
Copy link
Member

We might do 2.15.2 directly to save on labor cost.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0a6, 2.17.2 Apr 1, 2023
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Apr 3, 2023
Merged
@Pierre-Sassoulas Pierre-Sassoulas merged commit 1fa16c2 into pylint-dev:main Apr 3, 2023
@github-actions github-actions bot mentioned this pull request Apr 3, 2023
Merged
github-actions bot pushed a commit that referenced this pull request Apr 3, 2023
@jacobtylerwalls @github-actions
Add regression test for #7506 (#8432)
29006bf 
(cherry picked from commit 1fa16c2)
@Pierre-Sassoulas
Copy link
Member

Opened #8530 for the astroid upgrade.

@Pierre-Sassoulas
Copy link
Member

But ti's definitely not saving on labor cost because it create a conflict that prevent automatic backport 😄

Pierre-Sassoulas pushed a commit that referenced this pull request Apr 3, 2023
@jacobtylerwalls @Pierre-Sassoulas
Add regression test for #7506 (#8432)
136c571 
(cherry picked from commit 1fa16c2)
@jacobtylerwalls jacobtylerwalls deleted the issue-7506 branch April 3, 2023 11:31
Pierre-Sassoulas added a commit that referenced this pull request Apr 3, 2023
@github-actions @Pierre-Sassoulas @jacobtylerwalls
[Backport maintenance/2.17.x] Add regression test for #7506 (#8531)
84c197d 
* Add regression test for #7506 (#8432)

(cherry picked from commit 1fa16c2)

---------

Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Jacob Walls <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Assignees
No one assigned
Labels
Backported Regression
Projects
None yet
Milestone
2.17.2
Development

Successfully merging this pull request may close these issues.

False positive of unused-import since v2.15.0
2 participants
@jacobtylerwalls @Pierre-Sassoulas