elpy-importmagic-add-import should only add imports

[whirm]

Right now it reorders and deletes unused imports too.

This gets annoying sometimes when It deletes imports I was about to use and also breaks working code when some modules need to be imported in a specific way and importmagic changes its order or removes a module which is not directly used but needs to be imported anyway (IE twisted's reactor gets installed when imported for the first time)

I like having C-c C-r a to cleanup/reorder commits as an explicit task so I can see the diff before aplying in case I have to undo part of it. and C-c (S-)RET to just add one or all missing imports.

What do you think?

[jorgenschaefer]

Ideally, I'd like to have one command that "fixes imports in the current file", not two. There are two things to this I'd like it to do eventually, not necessarily related.

First, it should not remove imports that have a # noqa entry on the same line. flake8 uses this marker to not complain about problems, e.g. unused imports, so its a good marker for the user to explicitly say that this import is deliberate, even if it looks odd.

This is likely somewhat easy to add to the current importmagic code and would probably solve your problem, too, right?

Second, I'd like the single "fix up imports" command to pop up a diff buffer, let the user modify the diff as needed, and then apply it using C-c C-c. The current refactoring interface sadly does not do this well, either. I'm not sure yet how to implement this interface, but it's on my list for all refactoring options.

Do you think this would be a good idea?

[birkenfeld]

The "add-import" command (C-c RET) of the importmagic integration should not remove imports. If it does, that is a bug and can be fixed. (It does reorder them, but I don't consider that a bug.)

I would like to keep the "fixup" command doing what it does, though, since the refactoring command (C-c C-r) is deprecated. Adding a preview-and-modify feature is of course nice.

Recognizing # noqa or similar comments (e.g. a # pylint: disable=W... with the warning number for unused imports) is a good feature request. I also want to improve handling of "non-standard" import blocks, i.e. try-except or if-else conditional imports.

[whirm]

Hmm.. I think I may have mixed C-c RET with C-c S-RET during yesterday's tests as today I can't reproduce the import removal today with C-c RET. If I manage to reproduce it I'll post an update.

What I noticed is that the import sorting is way better when doing the fixup than with the deprecated tools.

This:

import gc
import logging
import os
import re
import shutil
import sys
import time
import unittest
from threading import enumerate as enumerate_threads
from traceback import print_exc

# set wxpython version before importing wx or anything from Tribler
import wxversion
wxversion.select("2.8-unicode")

import wx

from Tribler.Core import defaults
from Tribler.Core.Session import Session
from Tribler.Core.SessionConfig import SessionStartupConfig
from Tribler.Core.Utilities.twisted_thread import reactor

Gets sorted like this with the deprecated refactoring tools:

import gc
import logging
import os
import re
import shutil
import sys
import time
import unittest
from threading import enumerate as enumerate_threads
from traceback import print_exc

import wx
import wxversion

from Tribler.Core import defaults
from Tribler.Core.Session import Session
from Tribler.Core.SessionConfig import SessionStartupConfig
from Tribler.Core.Utilities.twisted_thread import reactor

# set wxpython version before importing wx or anything from Tribler
wxversion.select("2.8-unicode")

Which breaks the code as wx gets imported before selecting the right version.

There's a bug in the importmagic one though:

import gc
import logging
import os
import re
import shutil
import time
import unittest
from threading import enumerate as enumerate_threads
from traceback import print_exc

import wxversion

wxversion.select("2.8-unicode")

import wx

from Tribler.Core import defaults
from Tribler.Core.Session import Session
from Tribler.Core.SessionConfig import SessionStartupConfig
from Tribler.Core.Utilities.twisted_thread import reactor

It kills the comment line. (also leaves extra newlines behind, but that's minor)

[whirm]

Ideally, I'd like to have one command that "fixes imports in the current file", not two. There are two things to this I'd like it to do eventually, not necessarily related.

Yeah, C-c RET looks superfluous as if you have missing imports the code will be broken anyway :)

First, it should not remove imports that have a # noqa entry on the same line. flake8 uses this marker to not complain about problems, e.g. unused imports, so its a good marker for the user to explicitly say that this import is deliberate, even if it looks odd.

Maybe also a way to keep some imports in a fixed (relative) order? In a project i work, I use a non-default twisted reactor, which needs to be imported (gets installed at import time) before importing anything else twisted related so the default one doesn't get installed first. The thing is that this custom reactor is on the project's tree so it will always be put before the local modules when sorted.

Maybe just having a comment tag that makes them go first always would be enough.

This is likely somewhat easy to add to the current importmagic code and would probably solve your problem, too, right?

Well, I think it would be more impractical for my use case than re importing stuff that gets removed (one would have to go up, mark the stuff that doesn't have to be automatically cleaned up and then go back and continue coding)

I think there should be two methods: one that adds missing imports (and sorts the whole thing up) and another that cleans up unused stuff (skipping the # noqa marked imports).

This way no temporarily unused methods would be gone in the middle of an editing session and one could even hook the cleanup method to before-save-hook like a pro.

Second, I'd like the single "fix up imports" command to pop up a diff buffer, let the user modify the diff as needed, and then apply it using C-c C-c. The current refactoring interface sadly does not do this well, either. I'm not sure yet how to implement this interface, but it's on my list for all refactoring options.

The magit one is amazing, maybe you could just use that?

[jorgenschaefer]

Incidentally, there's also https://github.com/timothycrosley/isort for sorting imports, which I think I'd like to support eventually …

[alecthomas]

As @whirm is now aware (after writing a PR adding some TDD tests for it), there is a comment directive to force importmagic to manage only a specific block of imports:

# importmagic: manage
import os
import sys

All other imports should be ignored.

[jorgenschaefer]

Nice work!

[whirm]

@jorgenschaefer what does isort do that importmagic doesn't? Just sorting without cleaning unused imports or adding missing stuff?

[jorgenschaefer]

@whirm, good question, looks like it's a full subset :-D So, forget isort.