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

Add a RevOptions class #4707

Merged
merged 6 commits into from
Oct 1, 2017
Merged

Add a RevOptions class #4707

merged 6 commits into from
Oct 1, 2017

Conversation

cjerdonek
Copy link
Member

@cjerdonek cjerdonek commented Sep 2, 2017 •
edited
Loading

This PR adds a RevOptions class to encapsulate the argument information needed to run VCS commands for a revision.

Currently, the code base requires passing around at least three distinct, closely related pieces of information: the rev (revision to install), the rev_options (list of VCS command arguments), and rev_display (how to display the revision to the end-user). It would be easier if this information can be passed around as a single unit.

Also, in some cases the code "recomputes" the rev from the rev_options argument list. The Git class does this here:

return self.get_revision(dest).startswith(rev_options[0])

And here:

rev_options = self.check_rev_options(
    rev_options[0], dest, rev_options,
)

Accessing rev_options[0] is somewhat of a hack because it is brittle and VCS-specific. This can be done for the Git case because rev_options happens always to be [rev] (at least when rev_options is non-empty). For Subversion though, rev_options can take the form:

['-r', rev, '--username`, `John`]

The RevOptions class allows the rev to be accessed programmatically by storing this value as an attribute (e.g. letting us remove some Git-specific hacks like above).

Another advantage of adding a RevOptions class is that more code can be moved into a class that can be unit-tested in a stand-alone fashion (and without using mocks, etc). This PR adds such tests for the RevOptions class.

@cjerdonek
Copy link
Member Author

Also, this should be labeled: trivial, topic-vcs, and refactor.

@cjerdonek cjerdonek force-pushed the rev-options-class branch 2 times, most recently from b75e7c9 to c5f51f0 Compare September 2, 2017 19:44
@pradyunsg pradyunsg added type: refactor Refactoring code 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: maintenance Related to Development and Maintenance Processes and removed skip news Does not need a NEWS file entry (eg: trivial changes) labels Sep 3, 2017
@pradyunsg
Copy link
Member

pradyunsg commented Sep 3, 2017 •
edited
Loading

Whoops! I deleted my previous comment.

@cjerdonek It would be nice if you do:

/request-labels trivial topic-vcs refactor

It might be worth automating that if enough people start using that form. :)

As an aside, the trivial tag is for marking a PR as not needing a news file. :)

@cjerdonek
Copy link
Member Author

cjerdonek commented Sep 3, 2017 •
edited by pradyunsg
Loading

Okay, in the future, I'll do (as you suggested in your deleted comment):

/request-labels trivial topic-vcs refactor

And thanks for the explanation of the trivial tag! In the future, I'll add the empty news file instead of requesting a trivial tag.

@pradyunsg
Copy link
Member

(I found the comment in my email; added to the previous comment)

@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 Sep 4, 2017
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 4, 2017
@cjerdonek
Copy link
Member Author

Rebased.

@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 Sep 30, 2017
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Sep 30, 2017
xavfernandez
xavfernandez approved these changes Oct 1, 2017
View reviewed changes
src/pip/_internal/vcs/__init__.py Outdated

return self.rev

def to_args(self, start_args, end_args=None):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan:
I'd say cmd_args = ['branch', '-q'] + rev_options.to_args() + [url, dest] reads better.
An __add__ might be overkill.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a fair point. Thanks. I agree.

And thanks also for taking the time to review!

src/pip/_internal/vcs/__init__.py
self.extra_args = extra_args
self.rev = rev
self.vcs = vcs

Copy link
Member

Choose a reason for hiding this comment

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

Some basic __repr__ would be welcome.

src/pip/_internal/vcs/__init__.py
@@ -196,15 +285,19 @@ def check_version(self, dest, rev_options):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe update check_version's docstring also to stay consistent.

tests/unit/test_vcs.py Outdated
@@ -14,6 +15,85 @@
VERBOSE_FALSE = 0


def check_to_args(vcs, expected1, expected2):
Copy link
Member

Choose a reason for hiding this comment

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

This could be a pytest.mark.parametrize() test.

@xavfernandez xavfernandez merged commit 3e56733 into pypa:master Oct 1, 2017
@xavfernandez
Copy link
Member

Thanks for the nice refactor ! 👍

@cjerdonek
Copy link
Member Author

You're welcome, and thank you again for your help, @xavfernandez!

@cjerdonek cjerdonek mentioned this pull request Oct 3, 2017
Merged
@cjerdonek cjerdonek deleted the rev-options-class branch August 19, 2018 18:44
@lock
Copy link

lock bot commented Jun 1, 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 Jun 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xavfernandez xavfernandez xavfernandez approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation C: vcs pip's interaction with version control systems like git, svn and bzr type: maintenance Related to Development and Maintenance Processes type: refactor Refactoring code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cjerdonek @pradyunsg @BrownTruck @xavfernandez @pypa-bot