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

Reorder methods in req_install to better group them #5452

Merged
merged 2 commits into from
Jun 17, 2018

Conversation

pradyunsg
Copy link
Member

Toward #5051.

  • No code "changes".
  • Reorder the methods of InstallRequirement
    • Better grouping by what context they operate in -- source dist/wheel/editable/downloads only etc.
  • Also add a few comments and TODOs; mostly as pointers and hints for what to do next.

Getting the grouping of methods wrong here shouldn't really hurt much since this is intended to merely aid any work for #5051. :)

@pradyunsg pradyunsg added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels May 29, 2018
@pradyunsg pradyunsg self-assigned this May 29, 2018
@pradyunsg pradyunsg changed the title Reorder method in req_install to better group them Reorder methods in req_install to better group them May 29, 2018
Copy link
Member Author

@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.

On second thought, maybe moving it one method per commit would have been nicer. Then again, this doesn't really affect anything all that much so, it's probably okay.

@@ -129,6 +129,8 @@ def __init__(self, req, comes_from, source_dir=None, editable=False,
self.isolated = isolated
self.build_env = NoOpBuildEnvironment()

# Constructors
# TODO: Move these out of this class into custom methods.
Copy link
Member Author

Choose a reason for hiding this comment

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

obvious typo here: methods -> functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

(it'll get removed in #5455, so this staying in is not really a major issue)

def from_path(self):
"""Format a nice indicator to show where this "comes from"
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 doc-string slipped through. Letting this stay coz it adds value. :)

"""Find an installed distribution that satisfies or conflicts
with this requirement, and set self.satisfied_by or
self.conflicts_with appropriately.
"""
if self.req is None:
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 is just git anchoring the diff weirdly.

@pradyunsg pradyunsg added this to the Internal Cleansing milestone May 30, 2018
@pradyunsg pradyunsg closed this May 30, 2018
@pradyunsg pradyunsg reopened this May 30, 2018
@pradyunsg pradyunsg requested a review from a team May 30, 2018 17:27
@pradyunsg
Copy link
Member Author

a gentle ping @pfmoore @dstufft

@pradyunsg pradyunsg force-pushed the refactor/reorder-req-install-methods branch from 4277d04 to 47d58b8 Compare June 7, 2018 10:48
@pradyunsg
Copy link
Member Author

@pfmoore Could you take a look at this, when you find some time?

@pfmoore
Copy link
Member

pfmoore commented Jun 13, 2018

Not in the near future, sorry. I'm mad busy through till July, so only snatching bits of time till then, and I don't feel I can say anything meaningful on this based on the sort of quick skim that's all I can give it right now.

Sorry!

@pradyunsg
Copy link
Member Author

I'm mad busy through till July

Noted.

Sorry!

No issues. :)


I'm happy to merge this and a few more similar PRs, without someone else reviewing them, if no one else in @pypa/pip-committers has the time to review. Would you mind that?

@pradyunsg
Copy link
Member Author

This is basically a copy-paste change. I'm happy to merge w/o complete reviews into this.

@pradyunsg pradyunsg merged commit b41a712 into pypa:master Jun 17, 2018
@pradyunsg pradyunsg deleted the refactor/reorder-req-install-methods branch June 17, 2018 06:43
@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 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.

2 participants