Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --ignore-imports for multi-line imports #2424

Closed
wants to merge 10 commits into from
Closed

Conversation

@chkno
Copy link
Contributor

@chkno chkno commented Aug 19, 2018

This fixes #1422 and fixes #2019.

(I tried running the tox tests, but some of them seemed to be failing at HEAD, and I didn't have all the interpreters that it wanted to check installed on my machine.)

@coveralls
Copy link

@coveralls coveralls commented Aug 19, 2018

Coverage Status

Coverage increased (+0.007%) to 90.253% when pulling 19a14b8 on chkno:master into 664c712 on PyCQA:master.

tree = ast.fix_missing_locations(ast.parse(''.join(lines)))
line_is_import = {
node.lineno: isinstance(node, (ast.Import, ast.ImportFrom))
for node in tree.body}

This comment has been minimized.

@matthewfranglen

matthewfranglen Aug 20, 2018

This results in the final expression of the line overwriting any previous ones. For example:

from foo import bar; print (bar)

This results in a line_is_import of:

{1: False}

It's horrible code, however it is legal python. Something like this may work:

expression_is_import = [
    (node.lineno, isinstance(node, (ast.Import, ast.ImportFrom)))
    for node in tree.body]
line_is_import = {
    lineno: any(is_import for _, is_import in lineno_is_import)
    for lineno, lineno_is_import
    in groupby(expression_is_import, key=lambda expression: expression[0])}

This comment has been minimized.

@chkno

chkno Aug 20, 2018
Author Contributor

I see what you mean, but I think any should be all: --ignore-imports should only ignore imports; the presence of any code should keep the line under consideration.

Copy link
Member

@PCManticore PCManticore left a comment

@chkno Thank you so much for tackling this issue! This PR looks good, left just a couple of small improvements to be made. Also we'd need some boilerplate before merging:

  • please add yourself to CONTRIBUTORS file, if you are not already there
  • let's add a Changelog entry describing what this change fixes
@@ -18,8 +18,10 @@
"""

from __future__ import print_function
import ast

This comment has been minimized.

@PCManticore

PCManticore Aug 21, 2018
Member

Let's replace this with astroid instead. The API is similar with the ast module so you shouldn't need to modify anything other than the import.

This comment has been minimized.

@chkno

chkno Aug 21, 2018
Author Contributor

Done.

@@ -152,7 +164,9 @@ def stripped_lines(lines, ignore_comments, ignore_docstrings, ignore_imports):
docstring = None
line = ''
if ignore_imports:
if line.startswith("import ") or line.startswith("from "):
if lineno in line_is_import:

This comment has been minimized.

@PCManticore

PCManticore Aug 21, 2018
Member

These two lines could be rewritten as current_line_is_import = line_is_import.get(lineno)

This comment has been minimized.

@chkno

chkno Aug 21, 2018
Author Contributor

It can't: When lineno is not in line_is_import, current_line_is_import needs to keep its previous true/false value. Many lines are not present in line_is_import. I those cases, we must continue to use the value from the previous line.

current_line_is_import = line_is_import.get(lineno, current_line_is_import) would work, but this doesn't read as clearly to me.

I renamed line_is_import to line_begins_import to try to clarify this.

This comment has been minimized.

@PCManticore

PCManticore Aug 22, 2018
Member

Oh gotcha, this makes sense then. But I definitely suggest squashing this into line_is_import.get(lineno, current_line_is_import) as there is no reason to do the lookup and the retrieval separately.

This comment has been minimized.

@chkno

chkno Aug 23, 2018
Author Contributor

Um, ok. Done.

(If you don't mind making this a teaching moment: Why? I thought it might be for performance, but I measured and found that using .get() is slower (p=0.002, 95% CI slower by 130 - 570 ns/line) in cpython 3.6 (it trades a LOAD_FAST, COMPARE_OP, POP_JUMP_IF_FALSE, and BINARY_SUBSCR for a LOAD_ATTR, CALL_FUNCTION, and whatever the function does). Is it expected to be faster in pypy and people who care about performance run pylint with pypy, or is this just a python idiom I need to absorb? Thanks!)

This comment has been minimized.

@PCManticore

PCManticore Sep 29, 2018
Member

Oh, to be honest I was under the impression that this would have been faster than a lookup plus an additional access, put apparently that's not the case! So instead if was a teaching moment for me. :) Nonetheless, reducing the two lines to one looks better to me.

@@ -90,6 +94,51 @@ def test_ignore_imports():
""".strip()


def test_multiline_imports():
sys.stdout = StringIO()

This comment has been minimized.

@PCManticore

PCManticore Aug 21, 2018
Member

This idiom seems like a perfect candidate for contextlib. redirect_stdout

This comment has been minimized.

@chkno

chkno Aug 21, 2018
Author Contributor

Yes, it does. I'd like to keep this pull request narrowly focused on --ignore-imports, so I sent a separate pull request with this change: #2432

@chkno
Copy link
Contributor Author

@chkno chkno commented Aug 21, 2018

I updated Changelog and CONTRIBUTORS. (I saw these instructions in doc/development_guide/contribute.rst, but I wasn't sure if they were intended for a change this small. Thank you for walking me through this.)

(I rebased to resolve a Changelog merge conflict. Sorry if this was the wrong thing to do.)

@chkno chkno force-pushed the chkno:master branch from 743774b to 19a14b8 Aug 23, 2018
@PCManticore
Copy link
Member

@PCManticore PCManticore commented Sep 29, 2018

Thanks a lot for the PR @chkno ! I've merged this manually to solve the conflicts and just pushed the relevant commit to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.