Resolving linter errors flake8 #9
Conversation
Multiple linter errors resolved, reported by flake8.
|
Related to #1. |
| """Parser for SubRip. | ||
| """ | ||
| FORMAT = 'MicroDVD' | ||
| FORMAT_RE = re.compile(r'^\{(?P<start>\d+)\}\{(?P<end>\d+)\}(?P<header>(:?' |
There was a problem hiding this comment.
Try to understand the regex and break it into several components :).
| try: | ||
| h = h[1:-1] | ||
| # Take pair out, and strip them | ||
| d = dict([(j.strip() for j in i.split(':')) |
There was a problem hiding this comment.
Be more aggressive:
d = dict([
(
j.strip() for j in i.split(':')
)
for i in h.split('}{')
])
It would be probably nicer if you unnest it.
Resolving multiple comments from Github regarding coding style and additional flake8 linter errors.
| @@ -60,7 +61,7 @@ def export(self, output, subtitle): | |||
| raise TypeError("Can export only Subtitle objects.") | |||
|
|
|||
| try: | |||
| basestring | |||
| global basestring | |||
There was a problem hiding this comment.
I think it should be better to ignore flake8 error here.
Also, there might be a better way how to identify python 2/3 incompatibility for basestring.
There was a problem hiding this comment.
Some help with ignoring: http://flake8.pycqa.org/en/3.1.1/user/ignoring-errors.html
| @@ -52,7 +52,8 @@ def _export_unit(self, unit): | |||
| # TODO 3D positions | |||
| output.append("{} --> {}".format( | |||
| self._convert_time(unit.start), | |||
| self._convert_time(unit.end)).encode(self._encoding)) | |||
| self._convert_time(unit.end) | |||
There was a problem hiding this comment.
Last line in lists, tuples etc. should end with a comma. Helps when you add or move lines up and down (they look all the same).
| @@ -68,8 +67,8 @@ def __init__(self, stop_level='error'): | |||
| self._current_line = None | |||
|
|
|||
| def _add_msg(self, level, line_number, column, line, description): | |||
| if (self._stop_level and self.LEVELS.index(level) >= | |||
| self.LEVELS.index(self._stop_level)): | |||
| if (self._stop_level and self.LEVELS.index(level) | |||
There was a problem hiding this comment.
I think it would be more readable if breaking at and.
We have the first part of condition to safeguard so we don't use undefined _stop_level in the second part of the criteria. Other way how to solve this is, to ensure _stop_level always have a value, so you get a simpler condition.
Also, if you don't understand the condition at the beginning, try to understand it and give it a name. If you can, put it into a variable with that name. For this case, it could be named is_suppressed_message and use it as if not is_suppressed_message. Of course, you need to flip to condition for this.
Just some ideas for improving legacy code :).
| encoding = encodings.pop() | ||
| if can_decode(data, encoding if not isinstance(encoding, tuple) else | ||
| encoding[0]): | ||
| encoding = encoding_to_use = encodings.pop() |
There was a problem hiding this comment.
Naming could be better.
For instance:
encoding_with_confidence = encodings.pop()
if isinstance(encoding_with_confidence, tuple):
encoding = encoding_with_confidence[0]
else:
encoding = encoding_with_confidence
encoding_with_confidence = (encoding, None)There was a problem hiding this comment.
Just a small FYI (not to fix it, just to avoid code like this in future). I was here reusing encoding input parameter. It should be better to be named as default_encoding, it is less error prone.
| t['styles']['*']['font-size'] = v.strip() + \ | ||
| ('px' if v.strip().isdigit() else '') | ||
| t['styles']['*']['font-size'] = v.strip() | ||
| + ('px' if v.strip().isdigit() else '') |
There was a problem hiding this comment.
Are you sure this code works? I think you need \ or wrap whole expression into ().
Multiple linter errors resolved, reported by flake8.