Use pep8 logical lines checker

[jayvdb]

The pep8 logical lines checker API provides tokens for the logical line and noqa for the entire logical line.

Closes #10.

[jayvdb]

I'm intending to add a few more tests to this, but want to get your opinion first since it is such as such a large rewrite.

This has one 'false' positive I am aware of, and I am keen to solve it.

The following is valid syntax only on Python 3

def print():
    pass

class Foo(object):

    def print(self):
        pass

And not surprising, they cause a T001. IMO, at least the function should be a "T002: Python 2.x reserved word print used." or similar. using 'print' as a method name seems a) more likely to occur, and b) less problematic ; however it is still a syntax error on Python 2.

[JBKahn]

Great work by the way, just looking it over now. I'd never heard of the logical lines checker API, would have come in handy for sure.

Looks like fantastic work. I agree that we should try to deal with the false positive, I might get a chance to find out before you get back to me as I might look at this tonight. But what is the value of all of: token_type, text, start, end, line in the event of a false def print(self)...?

[JBKahn]

hmmmm

ipdb> print token_type, text, start, end, line
1 print (2, 4) (2, 9) def print(self):

the obvious possibility is checking that the line isn't a function call by doing a

line.lstrip().startswith('def ')

Would that cause a false positive?

[jayvdb]

I'll be able to resume this after work in about an hour and a half. the token 'def' followed by 'print' can be used to detect functions/methods fairly easily. Are you happy to have that be a new code; T002, or T101?

There are other false positives, such as def foo(print): and def foo(): print=1, as it can appear basically anywhere. Will need a decent set of tests to ensure they are being handled correctly. One option is to push the logical line through an ast parse to semantically understand the line so it is easy to handle it correctly.

[JBKahn]

ya, but T002 is taken by another one of my projects so maybe T101. That seems reasonable. when you catch print on a line, you can test instead for:

line.lstrip().startswith('def print')

As we know the text value is print so print isn't just part of the name in that case, it's the whole name?

But I've got nothing against using ast here, :+1: to that suggestion as well as the above one.

ooooo right. got it. hmmmm there are a fair amount of false positives. I think that maybe ast is the best solution then. It's late here so I clearly missed the point of that example the first time.

[JBKahn]

Two small questions about the regex's

for re.compile(r"(?<![=\s])\s*print\s+[^(=]", re.MULTILINE) What's purpose of having \s in the ![] followed by 0 or more \s?

for re.compile(r"(?<!def\s)print\s*\([^)]*\)", re.MULTILINE) Why does this not allow for \s* at the opening of the regex, are the logical_lines striped of all white-space in every case? And if so, are you sure? And if that's the case, why does the other regex have \s* before the keyword ?

One other question:

pos = logical_line.find('print')

Won't that flag every use of the word print, even inside a string or a comment? Or am I missing something? Do logical lines include comments and if not, what about strings?

[jayvdb]

pep8 strips comments and replaces string contents with xxx

[JBKahn]

Ahh, so then just the question about the regex. Otherwise I'll take one look at the tests again but lovely work.

[jayvdb]

Regarding whitespace, comments and strings being stripped, I am very confident. There is an overview at http://pep8.readthedocs.org/en/latest/developer.html#contribute , and I've also checked the pep8 source code that achieves those goals. e.g. the string 'xxx'ing is done here https://github.com/PyCQA/pep8/blob/master/pep8.py#L1463

One aspect I havent investigated is what is the minimum version of pep8 which works this way.

[JBKahn]

perfect, I just had the last question about what test would fail if \s was removed from the ![] so that I make sure I understand the regex fully. Otherwise I like all of it.

[JBKahn]

got it. two failures that I didn't think of: x = print if True else 1 and x = print or 1

Those were marked as print statement found :+1: I'll take a look at the tests after work tonight and if all is good on your end and I'm good, I'll merge it in.

[jayvdb]

Great. I'll try to think of more crazy cases. If you think of any, let me know. I'm not working today.

[jayvdb]

I've done a bit of investigation of these features in pep8, and they all appear to exist in very early versions (e.g. https://github.com/PyCQA/pep8/commit/3caaec04b74274e8c6bd000f5cf66c75319a2a0b from v1.0.1 in 2006) -- i.e. before flake8 existed, so I presume any version of flake8 will work correctly. I'm about 90% sure ;-)

[JBKahn]

we can always test it with a super early version of flake 8 or set a reasonable minimum version?

[jayvdb]

A small fix is needed for flake8 1.5 compatibility. But I have a power outage and cant upload it yet.

[JBKahn]

That sucks (the power outage I mean). No hurry, we can always get it out tomorrow.

On Tue, Nov 10, 2015, 7:34 PM John Vandenberg notifications@github.com wrote:

A small fix is needed for flake8 1.5 compatibility. But I have a power outage and cant upload it yet.

— Reply to this email directly or view it on GitHub https://github.com/JBKahn/flake8-print/pull/12#issuecomment-155612667.

[jayvdb]

(bah; ignore that one. I need to fix the skip logic)

[jayvdb]

Ready for another code review. All* green on a much larger matrix of old flake8 versions: https://travis-ci.org/jayvdb/flake8-print/builds/90454833 '*' Except an interesting incompatibility wrt Python 3.5 vs pep8 1.5-1.7, which is so extremely unlikely/undesirable combination I doubt anyone else will ever encounter it, and given pep8 was playing with globals() I am pretty confident the cpython 3.5 is doing the 'right' thing.

[JBKahn]

preliminary result looking good. I'm pretty sleepy though so I'll give it a final CR tomorrow and merge if it's a :+1:

[JBKahn]

Sorry, got in at 12:00 last night. Will handle that this today.

[JBKahn]

One more test maybe?

    @skipIfNoQAUnsupported()
    def test_skips_noqa_multiline(self):
        result = check_code_for_print_statements(dedent("""
            print("a"
                  "b"  # noqa
                  "c")
        """))
        assert_equal(result, list())

for what happens when it is neither the opening or closing bracket?

[JBKahn]

Otherwise I'm :+1:

[jayvdb]

Cant hurt ;-)

[JBKahn]

:+1: if you are. I'll give it a quick read and probably merge tonight if you're :+1:

[jayvdb]

it is good enough to merge as far as I am concerned, but happy to amend, especially adding more tests, if you find something that could be problematic.

No doubt there will be some strange case we haven't thought about...I'll be happy to help with any bug reports after it is released.

[JBKahn]

Alright, I can get behind that. I'll merge it now and install it locally tomorrow and play around with it for a day or two on my own code and then release it if all is good.