Disable import sorting for virtualenv

pikeas commented 4 years ago

Environment data

VS Code version: 1.49.2
Extension version (available under the Extensions sidebar): 2020.9.111407
OS and version: macOS 10.15.6
Type of virtual environment used: poetry
Value of the python.languageServer setting: Pylance

Expected behaviour

isort shouldn't run. Because the extension passes file contents via stdin, isort can't know whether the file should be ignored: https://github.com/PyCQA/isort/issues/1500

More generally, it's counterintuitive that 3rd party files are treated the same as project files. These files are sometimes viewed or edited to aid in debugging, but that shouldn't trigger formatting, import sorting, etc.

Actual behaviour

isort is run.

Output pane:

> <...>/.venv/bin/isort - --diff
cwd: <...>/.venv/lib/python3.8/site-packages/<package>

Steps to reproduce:

Open a file in the project's virtualenv site-packages directory and save it.

karrtikr commented 4 years ago

Can you paste your settings and configuration file (and it's location)? I assume you've some way of telling isort which files are to be skipped: https://pycqa.github.io/isort/docs/configuration/options/#skip

You can use "python.sortImports.args" to specify the settings path which contains this skip configuration as suggested here: https://github.com/PyCQA/isort/issues/1500#issuecomment-699583316

pikeas commented 4 years ago

@karrtikr Yes, I posted that issue. :-) That solution is a bit of a hack, it means that an additonal isort config file needs to be maintained just for use by the VS Code Python extension. I would say that by default, VS Code shouldn't sort imports that are inside a virtualenv.

Possible solutions for VS Code Python:

Pass the relative file path to isort instead of the file contents on stdin. This pattern is already implemented by the extension when it calls black for code formatting.
Add a python.sortImports.ignore setting.
Handle files in the active virtualenv's path differently, excluding them from most (all?) processing (code formatting, sorting, etc) by default.

karrtikr commented 4 years ago

it means that an additonal isort config file needs to be maintained just for use by the VS Code Python extension

Sorry I don't get this part, an additional isort config file? Can't you simply use the existing config file?

pikeas commented 4 years ago

No, because the only option that I can see is to create a second isort config file just for VS Code Python, which skips everything. And I'm not 100% sure that would work.

To be precise, the issue is that "skipping files" doesn't apply when processing stdin, which is what VS Code Python currently does. Is there a reason code formatters are called by filename but import sorting is called by stdin?

karrtikr commented 4 years ago

I don't see how it's related to whether isort being run as stdin or not, can you please explain? If we were using filePath to run isort, you'll still have to configure VSCode to let it know where the configuration file is.

Is there a reason code formatters are called by filename but import sorting is called by stdin?

Yes, I think it was so that isort works for unsaved files: https://github.com/microsoft/vscode-python/issues/4891

No, because the only option that I can see is to create a second isort config file just for VS Code Python, which skips everything.

Why though? I don't see why you can't simply point the setting path to the original isort config file (which I assume has this configuration to skip such files), without creating a second isort config file.

pikeas commented 4 years ago

Why though? I don't see why you can't simply point the setting path to the original isort config file (which I assume has this configuration to skip such files), without creating a second isort config file.

I think I see where the disconnect is. isort is run by multiple callers - by VS Code during development, as a commit hook, and during CI/CD. So any setting meant just for VS Code would impact all usage if in the main config, or require using a separate config just for VS Code.

Because VS Code calls isort via stdin, the existing settings for skipping files don't apply. There is no filename to match against, just the contents of stdin.

karrtikr commented 4 years ago

Oh I see what you mean, thanks for explaining! So currently you don't have a way to skip files if we're continuing with the stdin route.

One way we can avoid using stdin is if we go the old route & create a temporary file in the workspace folder as suggested here: https://github.com/microsoft/vscode-python/issues/4891#issuecomment-564472540 cc @PeterJCLaw do you see any problems with that?

PeterJCLaw commented 4 years ago

One way we can avoid using stdin is if we go the old route & create a temporary file in the workspace folder as suggested here: #4891 (comment) cc @PeterJCLaw do you see any problems with that?

The main one I can see is that such a temporary file would be inherently imperfect and while it would reduce the scope of this issue it cannot completely fix it. The reason being that the only valid equivalent name to the original file for isort to match on correctly is already taken -- by the original file.

Aside: there are other reasons to prefer using stdin for this sort of thing, most to do with filesystems being slow and/or possibly read-only; personally I'd rather more tools moved towards using stdin, though this issue does present an interesting challenge around that.

Perhaps I've missed it from the prior discussion -- what is triggering the isort run in this case? Is it being run manually? via the on-save functionality? If the user opens a file within a virtualenv and then manually triggers the isort command, then I think I would disagree about whether isort should run. The on-save behaviour is (IMO) still slightly questionable (and unlikely to be specific to isort), though I have more sympathy for this case.

Assuming this is the on-save behaviour, then a more general way to disable all formatting type handling for such files might be in order? Would you expect, for example, black to be run against these files?

Alternatively/additionally, if you really do want to use the same exclusion lists as isort's own configuration, perhaps a mechanism is need to co-operate with isort about which files to ignore. One option might be to pass isort the content of the file as well as the name of the file; another might be to ask isort first whether a file should be sorted; another might be to ask upfront for an allow-list of files in the project (and again after a new file is created/detected). I can imagine there may be tools which have much more per-file specific behaviour that might handle this already and we can take inspiration from. Flake8 for example has per-file-ignores; do they support something in this regard?

PeterJCLaw commented 4 years ago

Alternatively/additionally, if you really do want to use the same exclusion lists as isort's own configuration, perhaps a mechanism is need to co-operate with isort about which files to ignore.

It's since occurred to me that the ideal solution here is probably to run a Language Server Protocol façade in front of isort. Whether that's as a daemon or as a one-shot process probably doesn't matter too much, however by doing that and running isort via its Python API we could continue to use stdin while also conveying more than just the content of the file. This is approximately how the jedi integration works for example.

pikeas commented 4 years ago

Assuming this is the on-save behaviour, then a more general way to disable all formatting type handling for such files might be in order? Would you expect, for example, black to be run against these files?

I touched on this in an earlier response:

Handle files in the active virtualenv's path differently, excluding them from most (all?) processing (code formatting, sorting, etc) by default.

It's common to make a temporary change to a virtualenv file as part of development - logging, performance profiling, etc. This means saving the file, which triggers all of this extension's configured save actions, including formatting (black), linting (pylint), and import sorting (isort). Type checks (from Pylance) are also triggered on open, which is being discussed at https://github.com/microsoft/pylance-release/issues/422

My suggestion is that by default, virtualenv files should be treated differently from project files and not trigger any of these features.

PeterJCLaw commented 4 years ago

Thanks for the added detail. Yeah, I'm familiar with that sort of editing, though personally I don't tend to have formatters running on save.

My suggestion is that by default, virtualenv files should be treated differently from project files and not trigger any of these features.

Is there any reason to single out virtual environment files here specifically? Would we also expect to have similar treatment for python files in, for example, the node_modules folder (perhaps a JS project uses Python as a build step).

I've been wondering whether the solution here is that the automatic on-save actions should not be run for any files which appear in the projects exclusion lists (files.exclude, .gitignore if the related option is turned on, etc.)? This feels like it would be relatively unsurprising to users, is a list of exclusions they're going to maintaining anyway and fits within the notion that those exclusions exclude the files from editor behaviours (which the on-save operation essentially is).

Where I can see this falling down is for users who do still want their virtual environment files visible within the project, however those users are already going to get potentially unexpected results from e.g: find & replace so this would be consistent with that.

This could even be implemented at the editor level, so that extensions don't need to worry about it. Doing so has the benefit that it could be implemented once and (if a setting is needed) there would only be one setting. If there's a desire to override the behaviour per-language, then the usual "[language]": { setting ...} notation could easily be used.

HMate commented 3 years ago

We have a similar issue. We had to include the sources of an external library in our codebase, because we needed to change them.

Right now I also want ot run linting and formatting on the code when we save a file using isort and black. We want to exclude the source code of the external libary from formatting, so it will be easier to switch back to the original, once our use case is fixed. I can exclude the sources of the external library from black, with writing the library name in the force-exclude setting.

When isort is invoked from the command line with the filename, it finds the skip setting in out pyproject.toml file, and behaves correctly. The problem is that when saving a library file from vscode, the editor uses the stdinput with the file content, not the filepath, to invoke isort. This way isort does not know that it should skip the file.

I would appreciate it, if the solution to this issue would handle our use case too, so the filepath settings are respected by the built-in isort.

landonboyd commented 2 years ago

I have also encountered this issue. I have a number of exclusions specified in my isort configuration using skip_glob and filter_files. If isort knew what file it was working on (e.g. via the --filename option), it would be able to honour that configuration the same way it does when I run it from the command line.

ian-h-chamberlain commented 2 years ago

As mentioned above, isort supports the --filename arg for some time now, to allow usage like isort - --filename f.py to respect configuration like skip_globs.

See https://github.com/PyCQA/isort/issues/1596 and https://github.com/PyCQA/isort/blob/main/CHANGELOG.md#570-december-30th-2020

With this, it would be nice to be able to support something like this:

  "python.sortImports.path": "isort",
  "python.sortImports.args": [
    "--filter-files",
    "--skip-glob",
    "**/site-packages/**",
    "--filename",
    "${file}"
  ],

Unfortunately, the ${file} variable doesn't seem to be interpolated here, so as far as I can tell there's not a good manual workaround. It would probably nice if this argument was added automatically when using sortImports.py, or variables like ${file} and ${relativeFile} could be supported in sortImports.args for use with a custom sortImports.path.

I'd imagine that this --filename arg is really only useful when applied to a real file (not an unsaved file), so perhaps it's only really feasible to populate correctly when using the builtin sortImports.py.

Edit: I have figured out a hacky workaround! I wrote a little shell script to tack on the --filename arg based on first *.py file found in $PWD, which won't work all the time but works well enough for broad globs that end with e.g. **:

run_isort.sh:

#!/bin/bash

shopt -s nullglob

ARGS=("$@")
# Bit of a hack, but this just grabs the first *.py file and passes it as the
# filename. This allows us to use `--skip-globs` that cover sweeping directories,
# but might not work for e.g. `**/foo.py`.
FILE=("$PWD"/*.py)
if [[ ${#FILE[@]} -gt 0 ]]; then
    ARGS+=(--filename "${FILE[0]}")
fi

exec isort "${ARGS[@]}"

Then vscode config looks like this:

  "python.sortImports.path": "/path/to/run_isort.sh",
  "python.sortImports.args": [
    "--filter-files",
    "--skip-glob",
    "**/site-packages/**",
  ],

This works well enough for my use case, but it would be nice if this was supported properly by the extension as well.

alexreg commented 2 years ago

It would also be really nice to get a python.sortImports.enabled option. Right now, the slightly hacky solution to disable isort altogether is to add:

"python.sortImports.path": "true"

elpablete commented 2 years ago

It would also be really nice to get a python.sortImports.enabled option. Right now, the slightly hacky solution to disable isort altogether is to add:
"python.sortImports.path": "true"

This did not work for me in windows

alexreg commented 2 years ago

@elpablete That's because Windows doesn't have a program named true (unlike POSIX systems). Any program that does nothing and returns exit code 0 will do... maybe Windows has an equivalent, but I don't know because I don't use it.

elpablete commented 2 years ago

Thanks for the class. That true seemed weird to me so it is cool to actually learn what it means.

alexreg commented 2 years ago

@elpablete Yep, it's a bit odd... definitely a hack, but it works. Hopefully the authors of vscode-python will add a proper solution soon.

karrtikr commented 1 year ago

Can folks try out the isort extension and let us know if the issue still happens? https://marketplace.visualstudio.com/items?itemName=ms-python.isort

alexreg commented 1 year ago

As for my related issue (not the original one), there's also now a proper solution, I recently discovered. That is to add the following to your workspace settings.json to disable sorting imports on save. Or alternatively, put it in your global settings.json, and enable it per-project, which is what I do.

    "[python]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports": false,
        },
    },

karthiknadig commented 1 year ago

The isort extension checks for standard library files and skips them if they are not project files. Please use that: https://marketplace.visualstudio.com/items?itemName=ms-python.isort

You can also try a more performant variant, https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff

To enable isort on save, be sure to include "source.organizeImports.isort"

"[python]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports.isort": true,
        },
    },

To enable ruff on save, be sure to include "source.organizeImports.ruff"

"[python]": {
        "editor.codeActionsOnSave": {
            "source.organizeImports.ruff": true,
        },
    },

microsoft / vscode-python