Noissue - Added flake8 extension accounting for our coding style and some other stuff

Noissue - Added flake8 extension accounting for our coding style and some other stuff

[Sebastian Noack, 2016-04-21 19:02:12]

https://codereview.adblockplus.org/29340727/diff/29340731/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29340731/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:21: __version__ = '0.1'
It would be kinda cool if we could inject the version based on the mercurial
revision as a build step, given that we probably won't have releases for this
peace of code, and that it can only be used with flake8 after installing it via
setup.py, anyway.

However, I already spend more time than intended on this. But if you know from
the top of your head how to do that, or if you'd volunteer to look into that
later, that would be cool. ;)

[Vasily Kuznetsov, 2016-04-22 14:48:27]

Here are my comments on the version before the redundant parenthesis. I will
look at that check now.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:13: # You should have received a copy of the GNU
General Public License
Probably we should have some tests for it too. It should not be too difficult to
set up, just have one (or more if needed) code snipped that triggers each error.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:15: 
It would be good to describe how to use this extension with flake8. Maybe a
docstring here or a README file next to it. It seems that it's actually enough
to just install it and flake8 starts picking it up, but describing that would be
useful for people who don't know.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:28: BAILOUT = (ast.Return, ast.Raise, ast.Continue,
ast.Break)
Maybe something like LEAVE_BLOCK would be a better name?

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:52: self.stack = []
Perhaps it would be good to give this a more descriptive name. Like
`scopes_stack` or something similar. Otherwise `TreeVisitor` does a lot of
things and it's not clear what this stack is for.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:56: bailed = False
And here same as for BAILOUT, perhaps "left_block"?

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:75: if docstring and node is nodes[0] and
isinstance(node.value, ast.Str):
This line and A153 line are too long. Not by much though -- we should decide on
the max length.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:102: docstring=True)
E127 on this line, it should be under "node".

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:119: self.errors.append((node, 'A111 use format()
instead % operator '
Nit: "instead % operator" -> "instead of % operator"

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:128: self.errors.append((node, 'A112 use format()
instead + operator '
Same as above, "instead of + operator"

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:182: 
We could probably also watch for list/tuple/set literals in calls to
list/tuple/set here. If we are not allowing set([i for i in foo]), it makes
sense to also disallow set([1, 2, 3]).

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:205: self.errors.append((node, 'A141 global/nonlocal
declaration '
This complains at the following snippet:

t = 1

class A:
    global t
    t = 2

I'm not sure what could be the use case for such assignments to a global
variable from inside the class scope, but perhaps we shouldn't immediately
forbid it.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:214: self.errors.append((node, 'A159 redundant else
statement after '
This error is shown for the location of `if`. Would be better to show it on the
`else` instead.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:281: literal = re.sub(r'\\(?!{})'.format(literal[0]),
I think I still don't quite get the point of this but it seems to work right.
Why do we need to replace the quotes in raw strings?

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:295: yield (pos, "A311 string literal doesn't match
repr()")
It would be nice to explain in more detail what exactly is the problem here and
how it can be fixed. I can' come up with a nice short message though, so perhaps
this should be done in our style guide instead.

[Vasily Kuznetsov, 2016-04-22 15:50:04]

The if redundant parenthesis checker looks good. I tested it on some simple
cases and it's working without problems.

Another general question: how did you come up with the numbering for the errors?
Perhaps we should be a bit careful with the categories and the numbering in
general because these things will be a pain to change if, for example, we run
out of categories (A1...A9...oops) or something like that.

Perhaps the categorised list of errors should also go into the README to give an
easy overview.

[Sebastian Noack, 2016-05-07 17:14:04]

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:13: # You should have received a copy of the GNU
General Public License
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> Probably we should have some tests for it too. It should not be too difficult
to
> set up, just have one (or more if needed) code snipped that triggers each
error.

Done.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:28: BAILOUT = (ast.Return, ast.Raise, ast.Continue,
ast.Break)
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> Maybe something like LEAVE_BLOCK would be a better name?

Done.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:52: self.stack = []
On 2016/04/22 14:48:25, Vasily Kuznetsov wrote:
> Perhaps it would be good to give this a more descriptive name. Like
> `scopes_stack` or something similar. Otherwise `TreeVisitor` does a lot of
> things and it's not clear what this stack is for.

Done.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:56: bailed = False
On 2016/04/22 14:48:25, Vasily Kuznetsov wrote:
> And here same as for BAILOUT, perhaps "left_block"?

I went for leave_node, for consistency with pass_node, since we keep track of
the node that caused the block to be left for better error messages now.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:75: if docstring and node is nodes[0] and
isinstance(node.value, ast.Str):
On 2016/04/22 14:48:25, Vasily Kuznetsov wrote:
> This line and A153 line are too long. Not by much though -- we should decide
on
> the max length.

I made this line shorter by simply comparing against the loop index rather than
comparing this node against the first node.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:102: docstring=True)
On 2016/04/22 14:48:25, Vasily Kuznetsov wrote:
> E127 on this line, it should be under "node".

This is one of indentation errors I disagree with. IMO, it reads much better if
you have a single positional argument but multiple keyword arguments to align
the keyword arguments. In the case here it will even be aligned with the keyword
argument in the function call above. I wonder whether this is actually necessary
for compliance with PEP-8 or whether the pep8 utility is just too strict?

However, due to other changes the lambda function here is gone anyway.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:119: self.errors.append((node, 'A111 use format()
instead % operator '
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> Nit: "instead % operator" -> "instead of % operator"

Done.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:128: self.errors.append((node, 'A112 use format()
instead + operator '
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> Same as above, "instead of + operator"

Done.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:182: 
On 2016/04/22 14:48:27, Vasily Kuznetsov wrote:
> We could probably also watch for list/tuple/set literals in calls to
> list/tuple/set here. If we are not allowing set([i for i in foo]), it makes
> sense to also disallow set([1, 2, 3]).

Done. However, I intentionally ignore set literals as there is a legit use case
for list({x, y, z}), i.e. to exclude duplicated.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:205: self.errors.append((node, 'A141 global/nonlocal
declaration '
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> This complains at the following snippet:
> 
> t = 1
> 
> class A:
>     global t
>     t = 2
> 
> I'm not sure what could be the use case for such assignments to a global
> variable from inside the class scope, but perhaps we shouldn't immediately
> forbid it.

Done.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:214: self.errors.append((node, 'A159 redundant else
statement after '
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> This error is shown for the location of `if`. Would be better to show it on
the
> `else` instead.

Well, the position of else is unknown since the else block is part of the If
node. However, I changed it to indicate the position of the first node in the if
block.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:281: literal = re.sub(r'\\(?!{})'.format(literal[0]),
On 2016/04/22 14:48:25, Vasily Kuznetsov wrote:
> I think I still don't quite get the point of this but it seems to work right.
> Why do we need to replace the quotes in raw strings?

Well, the whole point of raw strings is that backslashes are interpreted
literally rather than escaping the next character (except when the next
character is a quote that needs to be escaped). So r'\ ' is equivalent to '\\ ',
and since repr() always uses the latter (non-raw) representation we have to
account for that.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:295: yield (pos, "A311 string literal doesn't match
repr()")
On 2016/04/22 14:48:26, Vasily Kuznetsov wrote:
> It would be nice to explain in more detail what exactly is the problem here
and
> how it can be fixed. I can' come up with a nice short message though, so
perhaps
> this should be done in our style guide instead.

Yes, that is something for the style guide. I will update it once this code has
been landed.

[Vasily Kuznetsov, 2016-05-09 13:39:52]

See my comments below.

It looks pretty good! I especially like the way the tests are defined now (and I
can also just run flake8 over those files for manual testing, or even load them
into vim+syntastic, which is pretty handly).

I still think it would be good to add a README file that explains what it is and
how it's meant to be used.

As for the error messages, they look good to me. Apart from some nitpicking
there's not much to add. I do think that we'll need some kind of document with a
more detailed explanation of the errors and our reasons for considering them
errors, but those lengthy explanations don't really belong to the error messages
themselves.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:102: docstring=True)
On 2016/05/07 17:14:03, Sebastian Noack wrote:
> On 2016/04/22 14:48:25, Vasily Kuznetsov wrote:
> > E127 on this line, it should be under "node".
> 
> This is one of indentation errors I disagree with. IMO, it reads much better
if
> you have a single positional argument but multiple keyword arguments to align
> the keyword arguments. In the case here it will even be aligned with the
keyword
> argument in the function call above. I wonder whether this is actually
necessary
> for compliance with PEP-8 or whether the pep8 utility is just too strict?
> 
> However, due to other changes the lambda function here is gone anyway.

Although I follow your logic and it's probably indeed more readable this way, in
the examples from PEP8 there is nothing like it
(https://www.python.org/dev/peps/pep-0008/#indentation). So perhaps makes sense
to just follow what pep8 utility says, it seems like not a big deal to me.
Anyway, not important anymore.

While we're at it, I just realised that PEP8 now recommends line breaks before
binary operators (see
https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after...)
but pep8 actually would not accept such code.

https://codereview.adblockplus.org/29340727/diff/29340762/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:182: 
On 2016/05/07 17:14:01, Sebastian Noack wrote:
> On 2016/04/22 14:48:27, Vasily Kuznetsov wrote:
> > We could probably also watch for list/tuple/set literals in calls to
> > list/tuple/set here. If we are not allowing set([i for i in foo]), it makes
> > sense to also disallow set([1, 2, 3]).
> 
> Done. However, I intentionally ignore set literals as there is a legit use
case
> for list({x, y, z}), i.e. to exclude duplicated.

Acknowledged.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:36: ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply',
'basestring', 'buffer',
What were the criteria for deciding what is an essential builtin? Maybe we
should document them somewhere. Otherwise it's not clear why, for example, it's
considered ok to redefine things like `unicode` and `basestring` but redefining
`id`, which is a useful name sometimes and is rarely used as a function, is not
acceptable.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:49: except Exception:
As I understand this exception would be thrown when there are external symbols
on which the node depends. Can it be anything other than NameError?
If not, maybe we should narrow down this exception to NameError?

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:246: '>2 strings'))
Maybe write it out: "more than two strings"? It's longer but I think a bit more
readable.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:273: self.errors.append((node, 'A301 use {}() instead '
instead -> instead of

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:286: 'instead calling {}() with '
instead -> instead of

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:302: 'comprehension instead calling '
instead -> instead of

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:409: yield (start, "A110 string literal doesn't match
repr()")
Maybe a bit clearer: "string literal doesn't match its repr()". It's still not
self-sufficient of course, and it's not alone. We will probably need to explain
the rationale for most of these errors somewhere anyway, so I guess the
elaborate version will go into that document.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/setup.py
File flake8-abp/setup.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/setup.py...
flake8-abp/setup.py:31: class TestCommand(Command):
Hm, a custom test runner. I would have probably just used pytest with parametric
fixtures but your approach also works and there are fewer external dependencies.

In any case I really like how the tests themselves look. Just some Python code
with mark up for expected errors in the comments -- that's concise and easy to
understand and author.

[Sebastian Noack, 2016-05-09 13:45:30]

On 2016/05/09 13:39:52, Vasily Kuznetsov wrote:
> I still think it would be good to add a README file that explains what it is
and
> how it's meant to be used.

Argh, I actually wrote a README, just forgot to add it to mercurial when
updating the patch. There you go. I will address your other comments later.

[Vasily Kuznetsov, 2016-05-09 15:45:14]

The README looks good. I would say that it doesn't have enough explanation to
understand the reasons in all cases but it's enough for a start and we can see
where we want to put the detailed explanations later.

https://codereview.adblockplus.org/29340727/diff/29341033/flake8-abp/README.md
File flake8-abp/README.md (right):

https://codereview.adblockplus.org/29340727/diff/29341033/flake8-abp/README.m...
flake8-abp/README.md:45: * `A206`: `else` statement after block is left
Maybe "Extraneous else" (in reference to
https://www.youtube.com/watch?v=JVVMMULwR4s&t=4m53s) or even the full title of
the talk (Eschew the extraneous else).

[Sebastian Noack, 2016-05-09 16:49:13]

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:36: ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply',
'basestring', 'buffer',
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> What were the criteria for deciding what is an essential builtin? Maybe we
> should document them somewhere. Otherwise it's not clear why, for example,
it's
> considered ok to redefine things like `unicode` and `basestring` but
redefining
> `id`, which is a useful name sometimes and is rarely used as a function, is
not
> acceptable.

The names explicitly excluded were the builtins that have been removed in Python
3. Some of them must be defined in compatibility code, and others are already
discouraged in Python 2 and can just be ignored.

However, I figured that it might better to just ignore top-level module code
here to account for compatibility code. Also note that we already ignored class
level code to allow for class attributes that would conflict with builtins.
While only checking for built-in redefinition in methods now, I cut the list of
deprecated builtins that are allowed to override.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:49: except Exception:
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> As I understand this exception would be thrown when there are external symbols
> on which the node depends. Can it be anything other than NameError?
> If not, maybe we should narrow down this exception to NameError?

Yes, it can be one of a wide range (if not any) exception. Just a few examples
of expressions that the given node might hold, causing different exceptions:

0/0                 -> ZeroDivisionError
eval('3ewr*/+%')    -> SyntaxError
__import__('foo')   -> ImportError
hash([])            -> TypeError
int("x")            -> ValueError
'ä'.decode('ascii') -> UnicodeDecodeError
None.foo            -> AttributeError

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:246: '>2 strings'))
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> Maybe write it out: "more than two strings"? It's longer but I think a bit
more
> readable.

I had it spelled out initially, but the message is already quite long, and it
certainly sticks out when spelled out. However, I changed it back for now.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:273: self.errors.append((node, 'A301 use {}() instead '
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> instead -> instead of

Done.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:286: 'instead calling {}() with '
On 2016/05/09 13:39:50, Vasily Kuznetsov wrote:
> instead -> instead of

Done.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:302: 'comprehension instead calling '
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> instead -> instead of

Done.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:409: yield (start, "A110 string literal doesn't match
repr()")
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> Maybe a bit clearer: "string literal doesn't match its repr()". It's still not
> self-sufficient of course, and it's not alone. We will probably need to
explain
> the rationale for most of these errors somewhere anyway, so I guess the
> elaborate version will go into that document.

I think that would be inaccurate. A string (literal) doesn't have a repr(), it's
an independent function that can be called on a string.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/setup.py
File flake8-abp/setup.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/setup.py...
flake8-abp/setup.py:31: class TestCommand(Command):
On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> Hm, a custom test runner. I would have probably just used pytest with
parametric
> fixtures but your approach also works and there are fewer external
dependencies.
> 
> In any case I really like how the tests themselves look. Just some Python code
> with mark up for expected errors in the comments -- that's concise and easy to
> understand and author.

I'm not sure how that code would look like if using pytest, but I feel the way
these tests work (which we seem to agree on), we wouldn't reuse much of any test
framework's infrastructure, in particular if we want to avoid additional
boilerplate.

https://codereview.adblockplus.org/29340727/diff/29341033/flake8-abp/README.md
File flake8-abp/README.md (right):

https://codereview.adblockplus.org/29340727/diff/29341033/flake8-abp/README.m...
flake8-abp/README.md:45: * `A206`: `else` statement after block is left
On 2016/05/09 15:45:13, Vasily Kuznetsov wrote:
> Maybe "Extraneous else" (in reference to
> https://www.youtube.com/watch?v=JVVMMULwR4s&t=4m53s) or even the full title of
> the talk (Eschew the extraneous else).

Hehe, that talk is awesome. I saw it last year at EuroPython. Done.

[Vasily Kuznetsov, 2016-05-09 17:37:33]

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:36: ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply',
'basestring', 'buffer',
On 2016/05/09 16:49:11, Sebastian Noack wrote:
> On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > What were the criteria for deciding what is an essential builtin? Maybe we
> > should document them somewhere. Otherwise it's not clear why, for example,
> it's
> > considered ok to redefine things like `unicode` and `basestring` but
> redefining
> > `id`, which is a useful name sometimes and is rarely used as a function, is
> not
> > acceptable.
> 
> The names explicitly excluded were the builtins that have been removed in
Python
> 3. Some of them must be defined in compatibility code, and others are already
> discouraged in Python 2 and can just be ignored.
> 
> However, I figured that it might better to just ignore top-level module code
> here to account for compatibility code. Also note that we already ignored
class
> level code to allow for class attributes that would conflict with builtins.
> While only checking for built-in redefinition in methods now, I cut the list
of
> deprecated builtins that are allowed to override.

Yep, makes sense. It would be good to record it somewhere, at least in a comment
here. Or maybe this logic really belongs to the style guide.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:49: except Exception:
On 2016/05/09 16:49:11, Sebastian Noack wrote:
> On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > As I understand this exception would be thrown when there are external
symbols
> > on which the node depends. Can it be anything other than NameError?
> > If not, maybe we should narrow down this exception to NameError?
> 
> Yes, it can be one of a wide range (if not any) exception. Just a few examples
> of expressions that the given node might hold, causing different exceptions:
> 
> 0/0                 -> ZeroDivisionError
> eval('3ewr*/+%')    -> SyntaxError
> __import__('foo')   -> ImportError
> hash([])            -> TypeError
> int("x")            -> ValueError
> 'ä'.decode('ascii') -> UnicodeDecodeError
> None.foo            -> AttributeError

I see. I just got a bit confused by this being called VOLATILE and the following
is_const function. So actually VOLATILE means that it depends on the environment
in some way or perhaps even always causes error (but not our error). Yeah, I
guess it makes sense to just catch `Exception` then.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:246: '>2 strings'))
On 2016/05/09 16:49:11, Sebastian Noack wrote:
> On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > Maybe write it out: "more than two strings"? It's longer but I think a bit
> more
> > readable.
> 
> I had it spelled out initially, but the message is already quite long, and it
> certainly sticks out when spelled out. However, I changed it back for now.

I see. I don't feel too strongly about it, so up to you.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:409: yield (start, "A110 string literal doesn't match
repr()")
On 2016/05/09 16:49:12, Sebastian Noack wrote:
> On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > Maybe a bit clearer: "string literal doesn't match its repr()". It's still
not
> > self-sufficient of course, and it's not alone. We will probably need to
> explain
> > the rationale for most of these errors somewhere anyway, so I guess the
> > elaborate version will go into that document.
> 
> I think that would be inaccurate. A string (literal) doesn't have a repr(),
it's
> an independent function that can be called on a string.

So is it about the difference between a string and a string literal or about the
inaccuracy of calling `repr(foo)` "a repr() of foo"?

The first objection is technically right, the literal doesn't have a repr(),
it's just part of the program source code and what does have a repr() is the
string that is created when the program is executed. But I was thinking that
distinguishing the string from the literal that it's created from is a bit too
much for our purposes here.

The second one depends on your point of view. If you think strictly
object-orientedly then a repr() of foo would be `foo.repr()` and not
`repr(foo)`. But Python has some functional programming in it as well. I think
most people would understand what I mean by len() of foo (`len(foo)`) and
similarly repr() of foo seemed to be ok. Perhaps we should ask a third person to
see what makes more sense.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/setup.py
File flake8-abp/setup.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/setup.py...
flake8-abp/setup.py:31: class TestCommand(Command):
On 2016/05/09 16:49:12, Sebastian Noack wrote:
> On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > Hm, a custom test runner. I would have probably just used pytest with
> parametric
> > fixtures but your approach also works and there are fewer external
> dependencies.
> > 
> > In any case I really like how the tests themselves look. Just some Python
code
> > with mark up for expected errors in the comments -- that's concise and easy
to
> > understand and author.
> 
> I'm not sure how that code would look like if using pytest, but I feel the way
> these tests work (which we seem to agree on), we wouldn't reuse much of any
test
> framework's infrastructure, in particular if we want to avoid additional
> boilerplate.

Yeah, you could plug it into py.test and make it run the same test (compare
flake8 output with the comments) for a bunch of instances of a parametric
fixture (the test input file). I think the amount of code would be about the
same. So it could make more sense if you wanted to also have other kinds of
tests that would benefit from a testing framework and you'd want to run it all
together. But here it's not the case so I think your implementation is as good.

https://codereview.adblockplus.org/29340727/diff/29341033/flake8-abp/README.md
File flake8-abp/README.md (right):

https://codereview.adblockplus.org/29340727/diff/29341033/flake8-abp/README.m...
flake8-abp/README.md:45: * `A206`: `else` statement after block is left
On 2016/05/09 16:49:12, Sebastian Noack wrote:
> On 2016/05/09 15:45:13, Vasily Kuznetsov wrote:
> > Maybe "Extraneous else" (in reference to
> > https://www.youtube.com/watch?v=JVVMMULwR4s&t=4m53s) or even the full title
of
> > the talk (Eschew the extraneous else).
> 
> Hehe, that talk is awesome. I saw it last year at EuroPython. Done.

Yeah, me too.

[Sebastian Noack, 2016-05-11 10:50:10]

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:36: ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply',
'basestring', 'buffer',
On 2016/05/09 17:37:32, Vasily Kuznetsov wrote:
> On 2016/05/09 16:49:11, Sebastian Noack wrote:
> > On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > > What were the criteria for deciding what is an essential builtin? Maybe we
> > > should document them somewhere. Otherwise it's not clear why, for example,
> > it's
> > > considered ok to redefine things like `unicode` and `basestring` but
> > redefining
> > > `id`, which is a useful name sometimes and is rarely used as a function,
is
> > not
> > > acceptable.
> > 
> > The names explicitly excluded were the builtins that have been removed in
> Python
> > 3. Some of them must be defined in compatibility code, and others are
already
> > discouraged in Python 2 and can just be ignored.
> > 
> > However, I figured that it might better to just ignore top-level module code
> > here to account for compatibility code. Also note that we already ignored
> class
> > level code to allow for class attributes that would conflict with builtins.
> > While only checking for built-in redefinition in methods now, I cut the list
> of
> > deprecated builtins that are allowed to override.
> 
> Yep, makes sense. It would be good to record it somewhere, at least in a
comment
> here. Or maybe this logic really belongs to the style guide.

I think the general rule for the style guide should be:

  Don't override built-in names except for outdated built-ins

Note that compatibility code like

  try:
      ascii
  except NameError:
      ascii = repr

doesn't override builtins, since it only assigns to ascii (which is a built-in
in Python 3) if it doesn't exist. However, without actually running the code we
won't know. So we have to account for it by other means here. But it's not an
exception to the general rule.

As for what built-ins are considered essential see the Python documentation:
https://docs.python.org/2/library/functions.html#non-essential-built-in-funcs

However, while "file" isn't listed as non-essential builtin there, I consider it
one given the argumentation there. It's only useful for strict type checks which
you should rather avoid anyway. And using "file" as variable name is arguably
more readable than using "fp" or "handle".

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:409: yield (start, "A110 string literal doesn't match
repr()")
On 2016/05/09 17:37:32, Vasily Kuznetsov wrote:
> On 2016/05/09 16:49:12, Sebastian Noack wrote:
> > On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > > Maybe a bit clearer: "string literal doesn't match its repr()". It's still
> not
> > > self-sufficient of course, and it's not alone. We will probably need to
> > explain
> > > the rationale for most of these errors somewhere anyway, so I guess the
> > > elaborate version will go into that document.
> > 
> > I think that would be inaccurate. A string (literal) doesn't have a repr(),
> it's
> > an independent function that can be called on a string.
> 
> So is it about the difference between a string and a string literal or about
the
> inaccuracy of calling `repr(foo)` "a repr() of foo"?
> 
> The first objection is technically right, the literal doesn't have a repr(),
> it's just part of the program source code and what does have a repr() is the
> string that is created when the program is executed. But I was thinking that
> distinguishing the string from the literal that it's created from is a bit too
> much for our purposes here.
> 
> The second one depends on your point of view. If you think strictly
> object-orientedly then a repr() of foo would be `foo.repr()` and not
> `repr(foo)`. But Python has some functional programming in it as well. I think
> most people would understand what I mean by len() of foo (`len(foo)`) and
> similarly repr() of foo seemed to be ok. Perhaps we should ask a third person
to
> see what makes more sense.

Yeah, I disagree. But also note that on Python 3 this message would be "A110
string literal doesn't match its ascii()" now, which sounds even more awkward to
me. What do you think?

[Vasily Kuznetsov, 2016-05-11 14:09:18]

See my comments below.

A bit off topic: it would be nice to have a feature in Rietveld to be able to
mark a comment thread as resolved. In the longer reviews it would be quite
helpful at the end of the review to just go through the unresolved comment
threads and not need to recheck everything.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
File flake8-abp/flake8_abp.py (right):

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:36: ESSENTIAL_BUILTINS = set(dir(builtins)) - {'apply',
'basestring', 'buffer',
On 2016/05/11 10:50:09, Sebastian Noack wrote:
> On 2016/05/09 17:37:32, Vasily Kuznetsov wrote:
> > On 2016/05/09 16:49:11, Sebastian Noack wrote:
> > > On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > > > What were the criteria for deciding what is an essential builtin? Maybe
we
> > > > should document them somewhere. Otherwise it's not clear why, for
example,
> > > it's
> > > > considered ok to redefine things like `unicode` and `basestring` but
> > > redefining
> > > > `id`, which is a useful name sometimes and is rarely used as a function,
> is
> > > not
> > > > acceptable.
> > > 
> > > The names explicitly excluded were the builtins that have been removed in
> > Python
> > > 3. Some of them must be defined in compatibility code, and others are
> already
> > > discouraged in Python 2 and can just be ignored.
> > > 
> > > However, I figured that it might better to just ignore top-level module
code
> > > here to account for compatibility code. Also note that we already ignored
> > class
> > > level code to allow for class attributes that would conflict with
builtins.
> > > While only checking for built-in redefinition in methods now, I cut the
list
> > of
> > > deprecated builtins that are allowed to override.
> > 
> > Yep, makes sense. It would be good to record it somewhere, at least in a
> comment
> > here. Or maybe this logic really belongs to the style guide.
> 
> I think the general rule for the style guide should be:
> 
>   Don't override built-in names except for outdated built-ins
> 
> Note that compatibility code like
> 
>   try:
>       ascii
>   except NameError:
>       ascii = repr
> 
> doesn't override builtins, since it only assigns to ascii (which is a built-in
> in Python 3) if it doesn't exist. However, without actually running the code
we
> won't know. So we have to account for it by other means here. But it's not an
> exception to the general rule.
> 
> As for what built-ins are considered essential see the Python documentation:
> https://docs.python.org/2/library/functions.html#non-essential-built-in-funcs
> 
> However, while "file" isn't listed as non-essential builtin there, I consider
it
> one given the argumentation there. It's only useful for strict type checks
which
> you should rather avoid anyway. And using "file" as variable name is arguably
> more readable than using "fp" or "handle".

Ok, I think saying that builtins should be overridden except for non-essential
ones from Python docs and `file` is clear and short. In general I agree with all
your logic here.

https://codereview.adblockplus.org/29340727/diff/29341006/flake8-abp/flake8_a...
flake8-abp/flake8_abp.py:409: yield (start, "A110 string literal doesn't match
repr()")
On 2016/05/11 10:50:09, Sebastian Noack wrote:
> On 2016/05/09 17:37:32, Vasily Kuznetsov wrote:
> > On 2016/05/09 16:49:12, Sebastian Noack wrote:
> > > On 2016/05/09 13:39:51, Vasily Kuznetsov wrote:
> > > > Maybe a bit clearer: "string literal doesn't match its repr()". It's
still
> > not
> > > > self-sufficient of course, and it's not alone. We will probably need to
> > > explain
> > > > the rationale for most of these errors somewhere anyway, so I guess the
> > > > elaborate version will go into that document.
> > > 
> > > I think that would be inaccurate. A string (literal) doesn't have a
repr(),
> > it's
> > > an independent function that can be called on a string.
> > 
> > So is it about the difference between a string and a string literal or about
> the
> > inaccuracy of calling `repr(foo)` "a repr() of foo"?
> > 
> > The first objection is technically right, the literal doesn't have a repr(),
> > it's just part of the program source code and what does have a repr() is the
> > string that is created when the program is executed. But I was thinking that
> > distinguishing the string from the literal that it's created from is a bit
too
> > much for our purposes here.
> > 
> > The second one depends on your point of view. If you think strictly
> > object-orientedly then a repr() of foo would be `foo.repr()` and not
> > `repr(foo)`. But Python has some functional programming in it as well. I
think
> > most people would understand what I mean by len() of foo (`len(foo)`) and
> > similarly repr() of foo seemed to be ok. Perhaps we should ask a third
person
> to
> > see what makes more sense.
> 
> Yeah, I disagree. But also note that on Python 3 this message would be "A110
> string literal doesn't match its ascii()" now, which sounds even more awkward
to
> me. What do you think?

Yeah, I agree with you that the proposed Python 3 message sounds very awkward.
So to be consistent, let's leave the wording as is.

[Vasily Kuznetsov, 2016-05-11 15:22:24]

LGTM