Issue 29378736: NoIssue - Improves converters.py PEP8 compliance (Closed)
|
Created:
March 8, 2017, 11:57 a.m. by Jon Sonesen Modified:
March 17, 2017, 8:41 a.m. Reviewers:
Vasily Kuznetsov Base URL:
https://hg.adblockplus.org/cms Visibility:
Public. |
DescriptionNoIssue - Improves converters.py PEP8 compliance
Patch Set 1 : #
Total comments: 13
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #MessagesTotal messages: 8
Just to make it a bit easier to work on ;)
Hi Jon, Thanks for volunteering to do the clean up work :) I have a few suggestions about improving readability a bit but otherwise it looks good. Cheers, Vasily https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:79: raise Exception('Unexpected HTML tag "{}" inside a fixed string' Do you think it's ok that we're changing the quotes in the string? Why not just do "bla bla bla '{}' bla bla".format(foo, bar)? https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:143: return re.sub(r'.', This is also kind of hard to read but more importantly, this is some serious case of excessive regexp love! Matching r'.', seriously? We'd need to rewrite this, but not in this ticket, so it's ok for now :) https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:162: default, saved_attributes, fixed_strings = (self._attribute_parser This looks kind of hard to read. How about this instead? default, saved_attributes, fixed_strings = ( self._attribute_parser.parse(default, self._params['page']) ) https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:255: locale, new_url = self._params['source']\ PEP8 discourages line continuation with backslashes. Perhaps we could do something like this: locale, new_url = self._params['source'].resolve_link( url, self._params['locale'] ) or even this: source = self._params['source'] locale, new_url = source.resolve_link(url, self._params['locale']) what do you think? https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:260: post += ' hreflang="{}"'\ Maybe we can use continuation with parentheses instead? post += (' hreflang="{}"' .format(jinja2.Markup.escape(locale))) Same below.
Thanks for you comments, btw for the commit message should it be 'No Issue - <Message>' or 'NoIssue - <Message>'? https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:79: raise Exception('Unexpected HTML tag "{}" inside a fixed string' On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > Do you think it's ok that we're changing the quotes in the string? Why not just > do "bla bla bla '{}' bla bla".format(foo, bar)? Well, the otherway causes a style warning, however this may actually be a bug in flake8-abp in which case I should change it back and file an issue for flake8-abp. https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:143: return re.sub(r'.', On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > This is also kind of hard to read but more importantly, this is some serious > case of excessive regexp love! Matching r'.', seriously? We'd need to rewrite > this, but not in this ticket, so it's ok for now :) I agree here, but assumed it would require an issue ticket. https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:162: default, saved_attributes, fixed_strings = (self._attribute_parser On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > This looks kind of hard to read. How about this instead? > > default, saved_attributes, fixed_strings = ( > self._attribute_parser.parse(default, self._params['page']) > ) Yes that seems nicer, thank you! https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:255: locale, new_url = self._params['source']\ On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > PEP8 discourages line continuation with backslashes. Perhaps we could do > something like this: > > locale, new_url = self._params['source'].resolve_link( > url, self._params['locale'] > ) > > or even this: > > source = self._params['source'] > locale, new_url = source.resolve_link(url, self._params['locale']) > > what do you think? oh yeah, I though i replaced all the back slashes I had added..whoops! https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:260: post += ' hreflang="{}"'\ On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > Maybe we can use continuation with parentheses instead? > > post += (' hreflang="{}"' > .format(jinja2.Markup.escape(locale))) > > Same below. yesh
https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:79: raise Exception('Unexpected HTML tag "{}" inside a fixed string' On 2017/03/08 17:51:57, Jon Sonesen wrote: > On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > > Do you think it's ok that we're changing the quotes in the string? Why not > just > > do "bla bla bla '{}' bla bla".format(foo, bar)? > > Well, the otherway causes a style warning, however this may actually be a bug in > flake8-abp in which case I should change it back and file an issue for > flake8-abp. whoops, I was wrong plz disregard XD
Just one small thing left... https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:79: raise Exception('Unexpected HTML tag "{}" inside a fixed string' On 2017/03/08 17:55:25, Jon Sonesen wrote: > On 2017/03/08 17:51:57, Jon Sonesen wrote: > > On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > > > Do you think it's ok that we're changing the quotes in the string? Why not > > just > > > do "bla bla bla '{}' bla bla".format(foo, bar)? > > > > Well, the otherway causes a style warning, however this may actually be a bug > in > > flake8-abp in which case I should change it back and file an issue for > > flake8-abp. > > whoops, I was wrong plz disregard XD Yeah, the logic is that if the string has single quotes in it, the easiest way is to just use double quotes around it. Basically it's always possible to check what the "right" way is by doing repr(your_string) in Python 2 or ascii(your_string) in Python 3. https://codereview.adblockplus.org/29378736/diff/29378739/cms/converters.py#n... cms/converters.py:143: return re.sub(r'.', On 2017/03/08 17:51:57, Jon Sonesen wrote: > On 2017/03/08 17:31:44, Vasily Kuznetsov wrote: > > This is also kind of hard to read but more importantly, this is some serious > > case of excessive regexp love! Matching r'.', seriously? We'd need to rewrite > > this, but not in this ticket, so it's ok for now :) > > I agree here, but assumed it would require an issue ticket. Yeah, you're right. https://codereview.adblockplus.org/29378736/diff/29378963/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29378736/diff/29378963/cms/converters.py#n... cms/converters.py:282: converter = converter_class(self._params, key='includedata' This looks kind of awkward with just the closing parentheses on the second line. Seems that it was better with the second argument on the second line. What do you think?
https://codereview.adblockplus.org/29378736/diff/29378963/cms/converters.py File cms/converters.py (right): https://codereview.adblockplus.org/29378736/diff/29378963/cms/converters.py#n... cms/converters.py:282: converter = converter_class(self._params, key='includedata' On 2017/03/09 12:33:27, Vasily Kuznetsov wrote: > This looks kind of awkward with just the closing parentheses on the second line. > Seems that it was better with the second argument on the second line. What do > you think? Yep, will do
LGTM 
|
