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

Disallow isort 5.13.0 #9290

Merged

Conversation

jacobtylerwalls
Copy link
Member

Refs #9270

isort 5.13.0 pulls in many dev dependencies as runtime dependencies, some of which cause behavior differences in astroid. See logs.

Refs PyCQA/isort#2206

Copy link

codecov bot commented Dec 10, 2023

Codecov Report

Merging #9290 (7e3c735) into main (796eae3) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9290   +/-   ##
=======================================
  Coverage   95.81%   95.81%           
=======================================
  Files         173      173           
  Lines       18762    18762           
=======================================
  Hits        17976    17976           
  Misses        786      786           

@jacobtylerwalls jacobtylerwalls added the Skip news 🔇 This change does not require a changelog entry label Dec 10, 2023
@jacobtylerwalls
Copy link
Member Author

For instance, in astroid, test_identify_old_namespace_package_protocol() starts failing if yarg is present, passes when uninstalled.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

This is only for our tests right? That probably shouldn't leak to our dependencies as the tool itself still works?

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Dec 10, 2023

Well, one of the tests indicates runtime behavior changes in astroid, which is critical to pylint. So no, I don't think it's a matter of tests only.

@jacobtylerwalls
Copy link
Member Author

It's possible that 'yarg' is patching sys.path, which makes me nervous.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Sorry, missed that test! The others looked fine. Indeed, let's not allow this.

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.

Long term for this part of the code I thought about:

  • use ruff instead of isort internally ?
  • an optional dependency for isort/ruff like the one we have for spelling,
  • dump the check and tell user to use isort/ruff directly ?
    But of course fixing astroid is an option 😄

@jacobtylerwalls
Copy link
Member Author

But of course fixing astroid is an option 😄

baby steps! 👶 🩰

@jacobtylerwalls jacobtylerwalls merged commit 47410ad into pylint-dev:main Dec 10, 2023
28 of 30 checks passed
@jacobtylerwalls jacobtylerwalls deleted the disallow-isort-5-13 branch December 10, 2023 22:09
github-actions bot pushed a commit that referenced this pull request Dec 10, 2023
Refs #9270

(cherry picked from commit 47410ad)
jacobtylerwalls added a commit that referenced this pull request Dec 10, 2023
Refs #9270

(cherry picked from commit 47410ad)

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
Labels
Backported Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants