Noissue - Adapt quotes for compliance with our coding style in sitescripts

Noissue - Adapt quotes for compliance with our coding style in sitescripts

[Sebastian Noack, 2016-05-29 13:29:39]

All changes except for tox.ini have been performed automatically by this script:
https://gist.github.com/snoack/ca864fbab537782e574508e0368de033

I verified that the resulting bytecode isn't changing using pydiff.

[Vasily Kuznetsov, 2016-05-30 10:01:42]

https://codereview.adblockplus.org/29345242/diff/29345301/sitescripts/urlfixe...
File sitescripts/urlfixer/bin/topDomains.py (right):

https://codereview.adblockplus.org/29345242/diff/29345301/sitescripts/urlfixe...
sitescripts/urlfixer/bin/topDomains.py:45: if
re.search(r"['\"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.", domain):
I now get A110 on this line. It seems that converting the regex to single quotes
is not really possible without changing it or dropping r and getting something
like the ugly automatic conversion that you replaced.

So it seems we have several options:
1. Leave it as is and add a # noqa to the line or include it into the config,
2. Revert to more ugly machine-translated version,
3. Change it to r'[\'"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.'

The third one will change the bytecode but it won't change the meaning of the
regex because backslash-escaped quotes in regexes are the same as unescaped
quotes (most likely the original regex escaped the double quote just to include
it in a double-quote-delimited string).

The third option seems to be the most "right" one but it's also the most scary
since we won't have the assurance of bytecode not changing.

What do you think?

[Sebastian Noack, 2016-05-30 10:27:18]

https://codereview.adblockplus.org/29345242/diff/29345301/sitescripts/urlfixe...
File sitescripts/urlfixer/bin/topDomains.py (right):

https://codereview.adblockplus.org/29345242/diff/29345301/sitescripts/urlfixe...
sitescripts/urlfixer/bin/topDomains.py:45: if
re.search(r"['\"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.", domain):
On 2016/05/30 10:01:41, Vasily Kuznetsov wrote:
> I now get A110 on this line. It seems that converting the regex to single
quotes
> is not really possible without changing it or dropping r and getting something
> like the ugly automatic conversion that you replaced.
> 
> So it seems we have several options:
> 1. Leave it as is and add a # noqa to the line or include it into the config,
> 2. Revert to more ugly machine-translated version,
> 3. Change it to r'[\'"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.'
> 
> The third one will change the bytecode but it won't change the meaning of the
> regex because backslash-escaped quotes in regexes are the same as unescaped
> quotes (most likely the original regex escaped the double quote just to
include
> it in a double-quote-delimited string).
> 
> The third option seems to be the most "right" one but it's also the most scary
> since we won't have the assurance of bytecode not changing.
> 
> What do you think?

Well, there is also the fourth option to fix flake8-abp which I went for:
https://codereview.adblockplus.org/29345337/

Though, generally, I'd agree that this string should have been written as
r'[\'"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.' which while not the same string anymore
should theoretically behave exactly the same when used as regular expression.
But we still would have to retest the code.

Moreover, when using raw strings for something other than regular expressions,
moving backslashes around might not even be an option. So we have to relax
flake8-abp anyway.

[Vasily Kuznetsov, 2016-05-30 12:16:34]

LGTM

https://codereview.adblockplus.org/29345242/diff/29345301/sitescripts/urlfixe...
File sitescripts/urlfixer/bin/topDomains.py (right):

https://codereview.adblockplus.org/29345242/diff/29345301/sitescripts/urlfixe...
sitescripts/urlfixer/bin/topDomains.py:45: if
re.search(r"['\"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.", domain):
On 2016/05/30 10:27:18, Sebastian Noack wrote:
> On 2016/05/30 10:01:41, Vasily Kuznetsov wrote:
> > I now get A110 on this line. It seems that converting the regex to single
> quotes
> > is not really possible without changing it or dropping r and getting
something
> > like the ugly automatic conversion that you replaced.
> > 
> > So it seems we have several options:
> > 1. Leave it as is and add a # noqa to the line or include it into the
config,
> > 2. Revert to more ugly machine-translated version,
> > 3. Change it to r'[\'"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.'
> > 
> > The third one will change the bytecode but it won't change the meaning of
the
> > regex because backslash-escaped quotes in regexes are the same as unescaped
> > quotes (most likely the original regex escaped the double quote just to
> include
> > it in a double-quote-delimited string).
> > 
> > The third option seems to be the most "right" one but it's also the most
scary
> > since we won't have the assurance of bytecode not changing.
> > 
> > What do you think?
> 
> Well, there is also the fourth option to fix flake8-abp which I went for:
> https://codereview.adblockplus.org/29345337/
> 
> Though, generally, I'd agree that this string should have been written as
> r'[\'"_,<>:;!$%&/()*+#~]|^\.|\.$|\.\.' which while not the same string anymore
> should theoretically behave exactly the same when used as regular expression.
> But we still would have to retest the code.
> 
> Moreover, when using raw strings for something other than regular expressions,
> moving backslashes around might not even be an option. So we have to relax
> flake8-abp anyway.

I agree that moving the backslashes around is not always possible, so fixing
flake8-abp seems like the best solution.