-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add Subversion.get_vcs_version method #6390
Conversation
…rrently installed Subversion client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
In addition to my review comments, can you also follow the pattern in the following PR and add a test_ensure_svn_available()
test that runs only on Travis? #6139
That way we can be sure that your test that requires svn to be installed will be running in at least one test environment in CI.
@cjerdonek Just an FYI, I agree with all of your formatting suggestions. I tried to keep this function consistent with the one for Git (where most of these issues are present currently): pip/src/pip/_internal/vcs/git.py Lines 68 to 79 in 54b6a91
|
Yeah, I noticed that. The Git one can be fixed up later to be more in line with this one. |
@cjerdonek I think I've addressed all of your feedback. Thanks for the detailed responses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- looking good! A few more comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A few more (final?) comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more tiny things..
Thank you, @johnthagen! 💯 |
Thanks for the mentoring @cjerdonek! |
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. |
Relates to #6386
cc @cjerdonek