Skip to content
Closed
Changes from all commits
Commits
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
30 changes: 20 additions & 10 deletions distro.py
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ def __init__(self, f):
self._f = f

def __get__(self, obj, owner):
assert obj is not None, 'call {} on an instance'.format(self._fname)
assert obj is not None, 'call {0} on an instance'.format(self._fname)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Genuinely curious, does 2.6 not support assuming format indexes?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It doesn't. It's a 2.7 and up thing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct. See https://docs.python.org/2/library/string.html#format-string-syntax

Changed in version 2.7: The positional argument specifiers can be omitted, so '{} {}' is equivalent to '{0} {1}'.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for verifying this!

ret = obj.__dict__[self._fname] = self._f(obj)
return ret

Expand Down Expand Up @@ -925,16 +925,26 @@ def _lsb_release_info(self):
Returns:
A dictionary containing all information items.
"""
if not self.include_lsb:
cmd = ['lsb_release', '-a']
process = subprocess.Popen(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to pipe to devnull anymore and I can't find anywhere where stderr is used meaningfully unless we get an error. Can we move the processing of stderr until that one error branch? (Or better yet: pipe it to /dev/null and not process it at all!)

cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
stdout, stderr = process.communicate()
stdout, stderr = stdout.decode('utf-8'), stderr.decode('utf-8')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's continue to use sys.getfilesystemencoding() instead of assuming utf-8 here. Also remove the tuple pack and unpack. Doesn't make the code any more readable IMO.

code = process.returncode
if code == 0:
content = stdout.splitlines()
return self._parse_lsb_release_content(content)
elif code == 127: # Command not found
return {}
with open(os.devnull, 'w') as devnull:
try:
cmd = ('lsb_release', '-a')
stdout = subprocess.check_output(cmd, stderr=devnull)
except OSError: # Command not found
return {}
content = stdout.decode(sys.getfilesystemencoding()).splitlines()
return self._parse_lsb_release_content(content)
else:
if sys.version_info[:2] >= (3, 5):

@sethmlarson sethmlarson Dec 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Grab version_info = sys.version_info[:2] once and compare it instead of grabbing it fresh each time. Also now this function raises errors where it would previously swallow them? That probably will have unintended side-effects?

raise subprocess.CalledProcessError(code, cmd, stdout, stderr)
elif sys.version_info[:2] >= (2, 7):
raise subprocess.CalledProcessError(code, cmd, stdout)
elif sys.version_info[:2] == (2, 6):
raise subprocess.CalledProcessError(code, cmd)

@staticmethod
def _parse_lsb_release_content(lines):
Expand Down