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

Code reorg utils into utils module reduces complexity #4990

Merged
merged 9 commits into from
Mar 30, 2022

Conversation

matteius
Copy link
Member

@matteius matteius commented Mar 17, 2022

The issue

The pipenv/utils.py has grown to 2338 lines of code, which makes it unwieldy and disorganized. When trying to consider larger bugs and refactors this is problematic because of the logical complexity of navigating around and understanding what kind of util you are looking at. I would like to do something similar with core.py at some point as well, but this is my first iteration of proposing reorg'ing some of the code.

#4992

The fix

Move the existing utils.py code into a utils module organized by functionality type. This is a first pass at it, and I suspect there may be some tweaking to my naming conventions to make everyone happy. The only draw back I see to doing this is loosing some of the git annotations around this code, but ultimately that would happen whenever we get around to organizing the code better and I think good code organization is more important than legacy annotations which would still ultimately be in the git history.

The only modifications to code being moved are the following methods have been dropped due to lack of use:

  • escape_grouped_arguments + unit tests
  • parse_python_version + unit tests
  • add_to_set
  • sys_version
  • get_indexes_from_requirement
  • interrupt_handled_subprocess
  • rmtree

Also two unused code lines were removed pipenv/core.py because I had already run the test suite showing they were not used after my IDE tipped me off about that.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@matteius matteius force-pushed the code-reorg-utils-reduces-complexity branch 2 times, most recently from 094436e to f498abb Compare March 17, 2022 19:11
@matteius matteius force-pushed the code-reorg-utils-reduces-complexity branch from f498abb to bfc810e Compare March 17, 2022 19:16
Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

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

It's a big one, I'll need a few more passes to review all of it.

pipenv/utils/dependencies.py Show resolved Hide resolved
…rning to above its usage.

* Move these utils from internet since they are shell related and remove rm_tree as it pipenv uses the version from vistir.
@matteius matteius force-pushed the code-reorg-utils-reduces-complexity branch from 4958e01 to f6481f9 Compare March 18, 2022 23:43
@@ -0,0 +1 @@
Internal to pipenv, the utils.py was split into a utils module with unused code removed.
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: that in the fast-follow linting PR I added a new type process same as Pip has, and this news fragment is converted to a process type in the subsequent PR. I went back and forth on feature vs process for this one, but its not an end-user facing feature.

@oz123 oz123 added this to the 2022.04.X milestone Mar 23, 2022
@matteius matteius added the PR: awaiting-review The PR related to this issue is awaiting review by a maintainer. label Mar 28, 2022
@oz123 oz123 removed the PR: awaiting-review The PR related to this issue is awaiting review by a maintainer. label Mar 29, 2022
@oz123
Copy link
Contributor

oz123 commented Mar 29, 2022

@matteius I think this can be merged.
@frostming want to have a look at this giant PR?

@matteius
Copy link
Member Author

I am going to merge this because I need to modify a util based on a new issue report from today, and I am tired of keeping those changes in sync with this PR. It will also allow merging the linting PR.

@matteius matteius merged commit 3387881 into main Mar 30, 2022
@matteius matteius deleted the code-reorg-utils-reduces-complexity branch March 30, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants