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

Breakout constructors from InstallRequirement #5455

Merged
merged 3 commits into from
Aug 21, 2018
Merged

Breakout constructors from InstallRequirement #5455

merged 3 commits into from
Aug 21, 2018

Conversation

pradyunsg
Copy link
Member

Built on top of #5452.

The idea here is that the constructors compose of about ~200 LOC and moving them out of the class would make it clear what support functions (defined at the top-level of the module) are used by them; allowing for cleanup and making it easier to focus on the other methods.

@pradyunsg pradyunsg added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels May 30, 2018
@pradyunsg pradyunsg added this to the Internal Cleansing milestone May 30, 2018
@pradyunsg pradyunsg mentioned this pull request May 30, 2018
Merged
@pradyunsg pradyunsg changed the title WIP: Breakout constructors from InstallRequirement Breakout constructors from InstallRequirement Jun 6, 2018
@pradyunsg pradyunsg requested a review from a team June 6, 2018 07:35
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jun 13, 2018
@pradyunsg pradyunsg added needs rebase or merge PR has conflicts with current master and removed needs rebase or merge PR has conflicts with current master labels Jun 21, 2018
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 21, 2018
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 4, 2018
@pradyunsg
Copy link
Member Author

@pfmoore I'm thinking of rebasing this PR -- could you take a look and check if these changes would conflict with your PEP 517 work?

@pfmoore
Copy link
Member

pfmoore commented Aug 21, 2018

I don't think they'd clash too badly. The req_install.py changes are the most extensive, and they don't touch code I'm working on. So probably OK.

@pradyunsg
Copy link
Member Author

Just to be sure, you don't mind if I go ahead with this and will be fine dealing with the merge conflicts it causes?

TBH, I think they'll mostly be import related, which should be trivial to resolve since flake8 points out missing names and duplicate imports.

@pfmoore
Copy link
Member

pfmoore commented Aug 21, 2018

Yeah, I'm OK with it.

@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Aug 21, 2018
@pradyunsg
Copy link
Member Author

@pfmoore Thanks! ^.^

@pradyunsg
Copy link
Member Author

CI is happy with these changes.
I'm happy with these changes.

The rest of the refactoring for #5051 happens after the PEP 517 work.

@pradyunsg pradyunsg merged commit 2ce7b28 into pypa:master Aug 21, 2018
@pradyunsg pradyunsg deleted the distributions/breakout branch August 21, 2018 16:14
@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.
Reviewers

@pfmoore pfmoore pfmoore approved these changes

Assignees
No one assigned
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
Milestone
Internal Cleansing
Development

Successfully merging this pull request may close these issues.
4 participants
@pradyunsg @BrownTruck @pfmoore @pypa-bot