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

Move DistAbstraction and subclasses into a 'distributions' subpackage #6109

Closed
wants to merge 2 commits into from

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Jan 2, 2019

Built on top of #6060

Step 4 of #5051, creating a location for placing separated logic for source/wheel/installed distributions in a better way.

@pradyunsg pradyunsg added the type: refactor Refactoring code label Jan 2, 2019
@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jan 7, 2019
@@ -0,0 +1,25 @@
from pip._internal.distributions.source import SourceDistribution as _SrcDist
from pip._internal.distributions.wheel import WheelDistribution as _WhlDist
Copy link
Member

Choose a reason for hiding this comment

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

Why the aliases ?

Copy link
Member Author

Choose a reason for hiding this comment

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

_names were too long for linter to be happy and I don't want people to do from from pip._internal.distributions import XYZ -- mentioning Source/Wheel Distribution is okay for now.

Eventually, this file should be empty anyway. :)

@@ -0,0 +1,26 @@
class AbstractDistribution(object):
"""A base class that abstracts information about
Copy link
Member

@cjerdonek cjerdonek Jan 13, 2019

Choose a reason for hiding this comment

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

I think it's better to put any changes to moved code (e.g. class renames, method renames, etc) in commits separate from the move operation, because otherwise you can't really tell what changes are being made to the moved classes. Like above, there is an incomplete docstring, but it wasn't like that in the original.

Copy link
Member Author

Choose a reason for hiding this comment

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

Duly noted. I'll take care of this in the future. :)

@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 be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 23, 2019
@pradyunsg
Copy link
Member Author

Yea, not now. Later.

@pradyunsg pradyunsg closed this Feb 24, 2019
@lock
Copy link

lock bot commented May 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 28, 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 needs rebase or merge PR has conflicts with current master 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.

4 participants