New autoimport functionality doesn't honor PEP-8 grouping

[baco]

The PEP-8, says about imports' position and order:

Imports should be grouped in the following order:

1. Standard library imports.
2. Related third party imports.
3. Local application/library specific imports.

You should put a blank line between each group of imports.

Also, the same PEP states there should be blank lines between the imports section and the rest of the code (depending on the first sentence after the imports).

The auto-import functionality does neither. If I have the following python code:

print("Hello")

when I start typing in the next line, say os, as if I want something from the os stdlib module, and hit enter on the autocomplete option, the resulting code is:

import os
print("Hello")
os

not having the correct separation between the import section and the code.

Furthermore, suppose now I start over and autoimport exit from sys, the resulting code being:

from sys import exit
print("Hello")
exit

and after that I want to autoimport date from datetime, the resulting code is:

from sys import exit
from datetime import date
print("Hello")
exit
date

Not only still the import section not being separated at all from the code section, but also, inserting the from datetime import date in the wrong order, as all imports should be sorted alphabetically within the same import group (both imports being stdlib in this case).

[ccordoba12]

Hey @baco, thanks for reporting. The autoimport docs mention that you need to install the isort plugin to get what you want.

So please do that and let us know if it works for you.

[baco]

Thanks for the prompt answer.

It seems I have it installed, but has no effect:

$ python3 -mpip show pyls-isort
Name: pyls-isort
Version: 0.2.2
Summary: Isort plugin for python-lsp-server
Home-page: https://github.com/paradoxxxzero/pyls-isort
Author: Florian Mounier
Author-email: paradoxxx.zero@gmail.com
License: MIT
Location: /home/baco/.local/share/nvim/mason/packages/python-lsp-server/venv/lib/python3.10/site-packages
Requires: isort, python-lsp-server
Required-by: 

[bagel897]

You'd also need to format on save - perhaps we can look into some kind of combined functionality but the current workflow is

1. Code and imports get pushed to top
2. Save(run isort and/or black) and imports get formatted

I don't know if we want to include this in the pylsp plugin directly or rope. Rope has limited support for this and cannot depend on isort while the pylsp plugin can

[baco]

Mm-hmmm... That would be an issue to me. I don't have any automatic code-formatter running on my code. Hence, I will not be adding any “auto-format on save hook”. Besides, that would be the same effect of running isort my self by hand.

The code should remain as the developer is looking at, and don't be altered beyond that.

I definitely prefer linters in that matter, because they teach you when you write poorly formatted code, but they teach you, the don't do the work leaving you with code you didn't write.

Auto-import functionality is a controlled insertion, you are being informed of whats going to be modified in your code, and how. But it should do the work complete, insert in the right position, not relying on that I'll be running some third party tool afterwards. I understand if the functionality needs of such tool to be helped compute the changes but, as the functionality, make the necessary arrangements by yourself.

As a note, to mention that pyright doesn't rely on running isort afterwards in order to insert the automatic imported modules in the right position.

Also, isort, for some project-structure has really troubles identifying first-party packages, and positions them as third-party packages. Then, for such projects, it would be a major issue to have the “format on save” hook turned on for every save. Different is if auto-import positions the module where it computes it belongs, most times will get it right, but in these cases I mention, perhaps not; but either way, if it gets it right, nothing else to be done, but if it gets it wrong (because of isort not detecting the first-party package), I can still manually position it where it belongs and won't be moved afterwards on save.

[bagel897]

That makes sense. The simplest option would be to insert a newline in the textedit but a comprehensive solution requires either depending on isort or reimplementing it (which I don't want to do). So I think it'd need the following change:

1. When completion item is requested, we delay resolution of the textedit till resolve
2. Resolve calls isort and places it correctly

[ccordoba12]

Sounds good to me @bagel897. I'm also in favor of making this server depend on the isort plugin (or isort directly) to better match the autoimport functionality with users' expectations.

[lieryan]

rope has a PEP8-compliant import organizer (rope.refactor.importutils.ImportOrganizer), so if you want to implement this in a rope-only way, you can just call ImportOrganizer; most of the time, the behavior of ImportOrganizer matches black/isort when it's configured for PEP8, but IME, there are sometimes some differences in whitespace handling around the import blocks.

Unless you start with a code that already complies to some import ordering policy (such as PEP8), where to add an import is not a well defined thing, so in practice, unless you have an import organizer running all the time (either isort or rope's), automated insertion of imports is never going to be 100% reliable and predictable. If a user expects running autoimport to be able to add imports to the right place all the time, the user also need to accept that running autoimport can modify other import statements as well to match the import ordering policy, not just adding/modifying a single line (!). This surprising behavior means that it's not really appropriate for an autoimport library like Rope to decide to automatically run import sorter for the user by default. The user (or the IDE/pylsp) need decide what autoformatter/import sorting tools they want to use.

For context, rope's autoimport doesn't assume that your codebase complies to PEP8 or any particular import ordering policy, so it tries to minimize any changes to existing imports; it just tries to find an existing import line that it can tack on to (e.g. an existing from-import from the same module) or add the new import to the end of your top-level import list and require you to run an import organizer afterwards if you want a specific ordering.

isort is a much more popular/well known utility than rope's organize import, so it is probably the better tool as the default import sorter for pylsp as people would already be more familiar with how to use and configure them and isort is are already used in pre-commits of a lot of projects (rope is ). In projects that has a policy to use isort, rope's ImportOrganizer can still be useful for its ability to remove unused imports and for automated enforcement of a certain import style (e.g. only from-import, or only module import), but the final say on import ordering should always be left to isort.