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

Sorting of imports (isort) is different if the file has unsaved modifications #4891

Closed
PeterJCLaw opened this issue Mar 22, 2019 · 10 comments · Fixed by #9128
Closed

Sorting of imports (isort) is different if the file has unsaved modifications #4891

PeterJCLaw opened this issue Mar 22, 2019 · 10 comments · Fixed by #9128
Labels
area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. bug Issue identified by VS Code Team member as probable bug

Comments

@PeterJCLaw
Copy link

Environment data

  • VS Code version: 1.32.3 (have also seen this on older versions)
  • Extension version: 2019.2.5558 (have also seen this on older versions)
  • OS and version: Ubuntu 16.04.6
  • Python version: 3.5.2 (have also seen this on other versions)
  • Type of virtual environment used: venv
  • Relevant/affected Python packages and their versions: I currently have isort 4.3.15, though I've seen this on older versions

Expected behaviour

That when running the "Sort Imports" command on a file, the imports are sorted according to my setup.cfg config regardless of whether the file is currently saved or not.

Actual behaviour

When running the "Sort Imports" command on a file:

  • If the file is saved, my setup.cfg config is respected
  • If the file has modifications, my setup.cfg config is not respected

Steps to reproduce:

Given this config:

# setup.cfg
[isort]
length_sort = True
# demo.py
import os
import logging
import collections
  1. put setup.cfg and demo.py next to each other in a new folder
  2. open that folder as a workspace in VSCode
  3. open demo.py
  4. "Sort imports" within demo.py, it should be unchanged
  5. edit demo.py, without saving it
  6. "Sort imports" within demo.py, note that its imports have changed ordering to ignore the length_sort option

This reproduces with much more complex configs, and other options are also ignored, so I strongly suspect the setup.cfg file isn't being picked up as a whole if the file is modified.

@ghost ghost added the triage-needed Needs assignment to the proper sub-team label Mar 22, 2019
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Mar 25, 2019
@karrtikr karrtikr added bug Issue identified by VS Code Team member as probable bug feature-refactoring and removed triage labels Mar 25, 2019
@karrtikr karrtikr removed their assignment Mar 25, 2019
@PeterJCLaw
Copy link
Author

I'd be up for trying to fix this if that would help. Do you have any suggestions around how to approach this?

From what I can see the issue here is that we're asking isort to operate on a temporary file like /tmp/tmp-19006aGV59gsvAPHQ.py rather than on a file within the project workspace.

Would a suitable fix be to put the temporary file within the project workspace instead of within /tmp?

@DonJayamanne
Copy link

Dup of #2980

@PeterJCLaw PeterJCLaw changed the title Sorting of imports (isort) is different if the file is unsaved Sorting of imports (isort) is different if the file has unsaved modifications Dec 14, 2019
@PeterJCLaw
Copy link
Author

I don't believe that this is a duplicate of #2980 -- that issue seems to be about new, unsaved files (which presumably don't have a well-defined project nor path on disc). In contrast this issue is about unsaved modifications to existing files. I've updated the title to clarify that.

@PeterJCLaw
Copy link
Author

Interestingly it looks like isort now supports being passed the content of the target file on stdin. While this project has a comment about this not being broken, I'm guessing that that pre-dates it being fixed (PyCQA/isort#384).

In any case, having tested the behaviour using stdin seems to correctly find configuration from the current directory (or ancestors), so I think this is a better solution than moving the temporary file. This would likely also solve #2980 for isort.

@PeterJCLaw
Copy link
Author

My current plan for this is to extend the IProcessService such that it supports passing something usable as the stdin of the underlying process.

I tried doing this using the existing SpawnOptions.stdio and passing essentially [myReadableWithFileCotent, undefined, undefined], but that didn't seem to work. I got errors that the Readable I was passing wasn't suitable; I'm guessing because it didn't have an actual file descriptor. If I'm missing something about this error I think this approach would be the best though.

I'm currently working on passing through a (new) IBufferEncoder to the ProcessService everywhere, with the idea that we can either add an input?: string attribute to the SpawnOptions or to the actual exec function signature. Input on whether it would be preferable to have the input specified in the function signature directly or as part of the options would be welcome.

@PeterJCLaw
Copy link
Author

I'm currently working on passing through a (new) IBufferEncoder to the ProcessService everywhere

This turns out to be redundant -- I'd missed that Writable.write can accept a string and an encoding.

with the idea that we can either add an input?: string attribute to the SpawnOptions or to the actual exec function signature. Input on whether it would be preferable to have the input specified in the function signature directly or as part of the options would be welcome.

I've done this via changing SpawnOptions in #9128, happy to change the approach if desired.

@danisaza
Copy link

I believe that I'm experiencing the same issue that Peter originally brought up.

In particular, here's how the issue arises:

  • I open an existing file in my project
  • I make an edit to the file. For example, I can do the following:
    • I add a newline character between two imports
    • I remove the newline character that I just added
    • Note: At this point, the file contents are exactly the same as when I opened the project.
  • I save the file

Upon saving, my imports are re-ordered. The configurations in my settings.cfg file seems to be ignored.

If I were to commit the changes to git at this point, the diff would be a bit unwieldy, even thought the actual contents of the file were not changed before I saved.

Then, without making any changes to the file, I save it again. This completely reorders the imports to what they previously were. The configurations in my settings.cfg file are used.

So, in summary...

  • I make a trivial change to an existing file in the project
  • Saving completely reorders the imports without taking settings.cfg into account
  • Immediately saving again reorders the imports and takes settings.cfg into account.

My current workaround

To save, I have to press cmd + s twice in a row each time I save a file to ensure that my imports end up in the right order.

The effect on my work

It's definitely annoying, but it's not an earth-melting issue. I'm still able to get work done with VS Code. That said, because saving a file is such a frequent action, this annoyance comes up a lot.

@DonJayamanne
Copy link

@luabud @karrtikr Accidentally moved this, not sure where this issue belongs

@brettcannon brettcannon added area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. and removed feature-refactoring labels Apr 9, 2020
@shughes-uk
Copy link

shughes-uk commented Apr 13, 2020

If anyone wants a workaround, setting the location of your settings file (pyproject.toml/setup.cfg) via an argument to isort works well.

 "python.sortImports.args": [
        "--settings-path",
        "somewhere/setup.cfg"
    ]

@simmol
Copy link

simmol commented May 5, 2020

If anyone wants a workaround, setting the location of your settings file (pyproject.toml/setup.cfg) via an argument to isort works well.

 "python.sortImports.args": [
        "--settings-path",
        "somewhere/setup.cfg"
    ]

I have the same issue and the this workaround save my sanity :)
Thanks a bunch @shughes-uk

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-intellisense LSP-related functionality: auto-complete, docstrings, navigation, refactoring, etc. bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants