Noissue - undo accidental logic changes introduced by 78d85c096f9e

Noissue - undo accidental logic changes introduced by 78d85c096f9e

Repository: https://hg.adblockplus.org/cms
Base revision: 8f0abb31be39

COLLABORATOR=jon@adblockplus.org

[Vasily Kuznetsov, 2017-03-11 17:03:12]

Hi Jon!

I looked at what needs to be done to fix the CMS bugs caused by PEP8-ization
today. It was actually not completely simple so I'm uploading it here. Checked
on web.adblockplus.org, web.adblockbrowser.org and web.eyeo.com against an older
version -- seems like the result is the same.

Cheers,
Vasily

[Vasily Kuznetsov, 2017-03-11 17:08:05]

P.S. I added 'from __future__ import unicode_literals' at the top -- it was
needed to remove the unicode errors because of the differences between the
behaviors of % and .format. This does change the behavior around encoding
handling a bit, but since the testing on abovementioned websites produces no
errors and no differences, this might be seen as an improvement :)

[Vasily Kuznetsov, 2017-03-12 12:18:54]

Hi Wladimir,

Jon and I have thought that you might also want to comment on this change. Feel
free to move yourself to reviewers if you would like.

Cheers,
Vasily

[Wladimir Palant, 2017-03-12 16:02:22]

Wow, that's a remarkable number of regressions for a single style-only change.
Feel free to improve test coverage while on it.

https://codereview.adblockplus.org/29380555/diff/29380556/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29380555/diff/29380556/cms/converters.py#n...
cms/converters.py:83: if tag == 'fix':
For reference, I'm not a huge fan of removing every "unnecessary" else
statement. This function exemplifies the point nicely. It handles four mutually
exclusive scenarios, and before the changes you could tell that just by looking
at the overall structure. Now you have to realize that the first branch has a
raise statement in it and so execution won't continue. For the Python
interpreter both versions are identical, but for humans the current version of
the code is harder to read.

From what I can tell, PEP8 doesn't mandate replacing elif by if statement here.
Would also be a rather unpythonic rule, with Python being very much about making
structures obvious.

https://codereview.adblockplus.org/29380555/diff/29380556/cms/converters.py#n...
cms/converters.py:209: lambda match: '<{}{}>{}</{}>'.format(
Like with my other comment, by not using raw strings for regular impressions you
aren't making it any easier for the Python interpreter - yet humans will no
longer see what's going on. More importantly, people changing these regular
expressions will have to remember that they need to switch to raw strings when
adding backslashes.

[Vasily Kuznetsov, 2017-03-13 12:25:29]

> Wow, that's a remarkable number of regressions for a single style-only change.
> Feel free to improve test coverage while on it.

Yeah, that's a good point. I think we would benefit from landing this change
ASAP (and for now I'm just relying on rendering web.adblockplus.org with new and
old versions of CMS and comparing the result with `diff -r`) but I created a
ticket for measuring and improving the test coverage:
https://issues.adblockplus.org/ticket/4981

https://codereview.adblockplus.org/29380555/diff/29380556/cms/converters.py
File cms/converters.py (right):

https://codereview.adblockplus.org/29380555/diff/29380556/cms/converters.py#n...
cms/converters.py:83: if tag == 'fix':
On 2017/03/12 16:02:22, Wladimir Palant wrote:
> For reference, I'm not a huge fan of removing every "unnecessary" else
> statement. This function exemplifies the point nicely. It handles four
mutually
> exclusive scenarios, and before the changes you could tell that just by
looking
> at the overall structure. Now you have to realize that the first branch has a
> raise statement in it and so execution won't continue. For the Python
> interpreter both versions are identical, but for humans the current version of
> the code is harder to read.
> 
> From what I can tell, PEP8 doesn't mandate replacing elif by if statement
here.
> Would also be a rather unpythonic rule, with Python being very much about
making
> structures obvious.

I agree that applying strict "no unnecessary else's" rule here makes the logic
less clear. We could of course add returns to other branches and then not have
any elif's or else's at all, but it seems like the original structure was
actually better.

Unfortunately our flake-eyeo plugin is rather strict about this so we seem to
have shot ourselves in the foot a bit. I have created a ticket to deal with this
(https://issues.adblockplus.org/ticket/4980) but for now I will revert the `if`
and add an exception to `tox.ini`.

https://codereview.adblockplus.org/29380555/diff/29380556/cms/converters.py#n...
cms/converters.py:209: lambda match: '<{}{}>{}</{}>'.format(
On 2017/03/12 16:02:22, Wladimir Palant wrote:
> Like with my other comment, by not using raw strings for regular impressions
you
> aren't making it any easier for the Python interpreter - yet humans will no
> longer see what's going on. More importantly, people changing these regular
> expressions will have to remember that they need to switch to raw strings when
> adding backslashes.

I switched from raw strings to unicode strings to address the more strict
behavior of .format with respect to unicode. However, after I added `from
__future__ import unicode_literals` to the top, this is no longer necessary, so
I reverted it to a raw string, which indeed is usually better for regular
expressions.

Thanks for catching this.

[Wladimir Palant, 2017-03-13 13:43:55]

LGTM

[Jon Sonesen, 2017-03-13 14:16:45]

On 2017/03/13 13:43:55, Wladimir Palant wrote:
> LGTM

LGTM