Remove vestigial VersionControl.get_info() method.
#5614
Conversation
xavfernandez
added
the
skip news
src/pip/_internal/vcs/__init__.py
Outdated
| @@ -422,7 +414,7 @@ def get_url(self, location): | |||
| """ | |||
| Return the url used at location | |||
|
|
|||
| This is used in get_info() and obtain(). | |||
| This is used in obtain(). | |||
There was a problem hiding this comment.
I would add get_src_requirement().
There was a problem hiding this comment.
We can just remove the "used in" comment IMO. It's easy enough to understand what this method is for without having to mention which other methods use it.
(that can't be said about some other methods)
| show_stdout=False, | ||
| extra_environ={'LANG': 'C'}, | ||
| ) | ||
| match = _svn_url_re.search(output) |
There was a problem hiding this comment.
Can _svn_url_re now also be deleted?
| logger.debug('Output that cannot be parsed: \n%s', output) | ||
| return None, None | ||
| url = match.group(1).strip() | ||
| match = _svn_revision_re.search(output) |
There was a problem hiding this comment.
Can _svn_revision_re now also be deleted?
| @@ -31,34 +31,6 @@ class Subversion(VersionControl): | |||
| def get_base_rev_args(self, rev): | |||
| return ['-r', rev] | |||
|
|
|||
| def get_info(self, location): | |||
There was a problem hiding this comment.
For getting the URL, I wonder which is better, the logic being deleted here or the logic in get_url()? This logic looks a bit simpler FWIW. It's curious that it's different because they're both supposed to do the same thing.
There was a problem hiding this comment.
get_url() relies on _get_svn_url_rev() to do its dirty work. I think _get_svn_url_rev() is more complicated than get_info() because it attempts to read the URL from disk before falling back to spawning a svn info child process (which is all get_info() does).
There was a problem hiding this comment.
That was more or less my assessment, too. I wonder if we should consider simplifying svn's get_url() implementation in the future with something more like get_info() (but not for this PR, of course). get_info() is simpler because it relies only on svn's command API instead of what looks like parsing svn implementation details that seem to vary from version to version.
There was a problem hiding this comment.
I think the intention is that reading from disk is orders of magnitude faster than launching a child process, otherwise the original author wouldn't have bothered!
There was a problem hiding this comment.
It’s certainly possible. But without seeing a code comment, issue discussion, or asking the author, I’m not sure we can say for sure.
There was a problem hiding this comment.
I don't understand the appeal of replacing a fast and well-tested method with one that hasn't been used in several years and which unconditionally (and usually unnecessarily) spawns a child process. Pip is slow enough already and I'd rather withdraw this PR than be forced to make it even slower for the sake of removing a vestigial method.
There was a problem hiding this comment.
and I'd rather withdraw this PR than be forced to make it even slower for the sake of removing a vestigial method.
I wasn't suggesting that you change your PR. I said, "I wonder if we should consider simplifying svn's get_url() implementation in the future..." and "not for this PR, of course." It was an observation / idea. I approved your PR.
There was a problem hiding this comment.
Thanks for clarifying!
|
It looks like the last (only?) thing to call |
Yeah, I researched that, too, when I reviewed your change. Thanks for double-checking. |
I wasn't double-checking - I was putting this info on github for posterity. |
|
Okay, well thanks for posting it on GitHub then. I was just trying to express appreciation for taking the time to look into it. |
|
Thanks @barneygale! ^>^ |
|
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
bot
added
the
auto-locked
I can't see that this method is ever called.