Skip to content

Remove vestigial VersionControl.get_info() method. #5614

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

Merged
merged 2 commits into from
Jul 22, 2018
Merged

Remove vestigial VersionControl.get_info() method. #5614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
814b4bd
Remove vestigial `VersionControl.get_info()` method.
barneygale Jul 18, 2018
05eacbc
Address review feedback
barneygale Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions src/pip/_internal/vcs/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,14 +250,6 @@ def get_url_rev_options(self, url):

return url, rev_options

def get_info(self, location):
"""
Returns (url, revision), where both are strings
"""
assert not location.rstrip('/').endswith(self.dirname), \
'Bad directory: %s' % location
return self.get_url(location), self.get_revision(location)

def normalize_url(self, url):
"""
Normalize a URL for comparison by unquoting it and removing any
Expand Down Expand Up @@ -421,8 +413,6 @@ def get_src_requirement(self, dist, location):
def get_url(self, location):
"""
Return the url used at location

This is used in get_info() and obtain().
"""
raise NotImplementedError

Expand Down
30 changes: 0 additions & 30 deletions src/pip/_internal/vcs/subversion.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@

_svn_xml_url_re = re.compile('url="([^"]+)"')
_svn_rev_re = re.compile(r'committed-rev="(\d+)"')
_svn_url_re = re.compile(r'URL: (.+)')
_svn_revision_re = re.compile(r'Revision: (.+)')
_svn_info_xml_rev_re = re.compile(r'\s*revision="(\d+)"')
_svn_info_xml_url_re = re.compile(r'<url>(.*)</url>')

Expand All @@ -31,34 +29,6 @@ class Subversion(VersionControl):
def get_base_rev_args(self, rev):
return ['-r', rev]

def get_info(self, location):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

@cjerdonek cjerdonek Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying!

"""Returns (url, revision), where both are strings"""
assert not location.rstrip('/').endswith(self.dirname), \
'Bad directory: %s' % location
output = self.run_command(
['info', location],
show_stdout=False,
extra_environ={'LANG': 'C'},
)
match = _svn_url_re.search(output)
Copy link
Member

Choose a reason for hiding this comment

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

Can _svn_url_re now also be deleted?

if not match:
logger.warning(
'Cannot determine URL of svn checkout %s',
display_path(location),
)
logger.debug('Output that cannot be parsed: \n%s', output)
return None, None
url = match.group(1).strip()
match = _svn_revision_re.search(output)
Copy link
Member

Choose a reason for hiding this comment

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

Can _svn_revision_re now also be deleted?

if not match:
logger.warning(
'Cannot determine revision of svn checkout %s',
display_path(location),
)
logger.debug('Output that cannot be parsed: \n%s', output)
return url, None
return url, match.group(1)

def export(self, location):
"""Export the svn repository at the url to the destination location"""
url, rev_options = self.get_url_rev_options(self.url)
Expand Down