Conversation
Multiple linter errors resolved, reported by flake8.
|
Related to #1. |
MasterMind2k
left a comment
MasterMind2k
left a comment
There was a problem hiding this comment.
Looks very good for first try.
I suspect majority of work was done by autopep8?
| """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.
MasterMind2k
left a comment
MasterMind2k
left a comment
There was a problem hiding this comment.
Good work.
Some minor comments left.
Don't forget to rebase to latest master to get the changes from parallel branch in.
|
|
||
| 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
| 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).
| 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 ().
2 participants
Multiple linter errors resolved, reported by flake8.