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

Remove the module-level get_src_requirement(). #5881

Merged
merged 4 commits into from
Oct 22, 2018

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Oct 13, 2018

This is my last preparatory refactoring before posting a PR for issue #5031.

@cjerdonek cjerdonek added type: refactor Refactoring code C: vcs pip's interaction with version control systems like git, svn and bzr C: freeze 'pip freeze' related skip news Does not need a NEWS file entry (eg: trivial changes) labels Oct 13, 2018
@cjerdonek cjerdonek added this to the Internal Cleansing milestone Oct 13, 2018
@cjerdonek cjerdonek force-pushed the remove-vcs-get-src-requirement branch from 53c21ec to c976f2c Compare October 13, 2018 09:15
@cjerdonek
Copy link
Member Author

This PR removes the module-level get_src_requirement() function inside pip's vcs module.

There are a number of reasons for this:

  1. The function is used only once in the entire code base (in the get_requirement_info() method that this PR is modifying), so we have the ability to inline it.

  2. Inlining the function lets us DRY up the calls to dist.as_requirement() to a single place, which we do in this PR.

  3. Other than calling dist.as_requirement(), all the function does is add a logger.warning() call in a try-except.

  4. Moving the logging call to get_requirement_info() lets us see how the logging call fits in with the rest of the logging calls inside get_requirement_info() and puts us in a position to remove duplicate or similar logging calls (there are such) and adjust the comments accordingly.

  5. Inlining the function lets us combine the try-except block in get_src_requirement() with the try-except logic in get_requirement_info().

@cjerdonek cjerdonek changed the title [WIP] Remove the module-level get_src_requirement(). Remove the module-level get_src_requirement(). Oct 13, 2018
Copy link
Member

@pradyunsg pradyunsg 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 control flow changes here - they make things easier to follow and more readable.

@@ -164,27 +164,33 @@ def __init__(self, name, req, editable, comments=()):
self.comments = comments

@classmethod
def _init_args_from_dist(cls, dist, dependency_links):
def get_requirement_info(cls, dist, dependency_links):
Copy link
Member

@pradyunsg pradyunsg Oct 16, 2018

Choose a reason for hiding this comment

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

I don't think this needs to be a classmethod since it doesn't actually use cls. It can probably be a module level function.

@cjerdonek cjerdonek force-pushed the remove-vcs-get-src-requirement branch from 2a8311e to 46ffb13 Compare October 16, 2018 09:57
@cjerdonek
Copy link
Member Author

I don't think this needs to be a classmethod since it doesn't actually use cls. It can probably be a module level function.

@pradyunsg Good observation -- thanks! I've made that change.

When making that change, I noticed an additional simplification that can be made. Not only the cls argument, but also the dependency_links argument can be removed. This was made possible by PR #5869, but I didn't notice it then.

Does it still look okay to you now? Thanks again.

@cjerdonek
Copy link
Member Author

Thanks, @pradyunsg and @xavfernandez!

@cjerdonek cjerdonek merged commit 4e6bdf6 into pypa:master Oct 22, 2018
@cjerdonek cjerdonek deleted the remove-vcs-get-src-requirement branch October 22, 2018 08:45
@pradyunsg
Copy link
Member

Post merge approval from me. :)

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 24, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation C: freeze 'pip freeze' related C: vcs pip's interaction with version control systems like git, svn and bzr skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants