Ensure sorting imports in a modified file picks up the proper configuration

[PeterJCLaw]

Fixes #4891.

• Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
• Title summarizes what is changing
• Has a news entry file (remember to thank yourself!)
• Appropriate comments and documentation strings in the code
• Has sufficient logging.
• Has telemetry for enhancements.
• Unit tests & system/integration tests are added/updated
  Tests updated in importSortProvider.unit.test.ts and a regression test added to extension.sort.test.ts
• Test plan is updated as appropriate
• package-lock.json has been regenerated by running npm install (if dependencies have changed)
• The wiki is updated with any design decisions/details.

[codecov-io]

Codecov Report

Merging #9128 into master will decrease coverage by 0.36%.
The diff coverage is 82.05%.

@@            Coverage Diff             @@
##           master    #9128      +/-   ##
==========================================
- Coverage   61.08%   60.71%   -0.37%     
==========================================
  Files         601      629      +28     
  Lines       33085    34004     +919     
  Branches     4671     4791     +120     
==========================================
+ Hits        20209    20647     +438     
- Misses      11858    12350     +492     
+ Partials     1018     1007      -11     

Impacted Files	Coverage Δ
src/client/providers/importSortProvider.ts	81.81% <82.05%> (+1.20%)	⬆️
...cience/ipywidgets/cdnWidgetScriptSourceProvider.ts	15.51% <0.00%> (-68.80%)	⬇️
src/client/datascience/raw-kernel/rawKernel.ts	7.07% <0.00%> (-48.21%)	⬇️
src/client/datascience/jupyter/jupyterVariables.ts	25.00% <0.00%> (-24.18%)	⬇️
src/client/datascience/jupyter/jupyterUtils.ts	57.14% <0.00%> (-17.28%)	⬇️
src/client/common/process/types.ts	84.61% <0.00%> (-15.39%)	⬇️
src/datascience-ui/react-common/arePathsSame.ts	75.00% <0.00%> (-12.50%)	⬇️
src/client/common/utils/platform.ts	64.70% <0.00%> (-11.77%)	⬇️
...datascience/interactive-common/notebookProvider.ts	48.14% <0.00%> (-9.00%)	⬇️
src/client/common/process/pythonDaemon.ts	15.87% <0.00%> (-4.03%)	⬇️
... and 117 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1768483...b5528ee. Read the comment docs.

[karthiknadig]

@PeterJCLaw Thank you for the PR. Can you look at the merge conflicts?

[DonJayamanne]

@PeterJCLaw Thanks, for the PR, however I'd consider revising the approach.
@karthiknadig /cc
I'm not a fan of passing in intput into the process class (i think it ends up with too much responsibility).
If we're spawning a long running process, which is what execObservable is for, then the calling code has access to the process object and it can write directly into the stdin stream.

Else think of other CLIs, they might have muliple inputs. In such a situation the second input can be written into stdin stream only after the first one has been processed. This knowledge is available to the calling code.
Also, what if we need to pass in two lines into stdin (one after the other). For this we need to ensure the process is ready to access the input for the second item.
Considering this I'm not convinced we should be modifying processService.

I'd like to see this being done outside this class. Elsewhere, e.g. in some isort specific class.
To me the the responsibility of exeObservable is to spawn a process, return the process along with an output stream. If required, the calling code can write directly into the intput stream.

[DonJayamanne]

@karthiknadig /cc

[PeterJCLaw]

@DonJayamanne thanks for the feedback.

The change to execObservable here was in order to keep the same interface between it in exec, though I agree it's a little contrived. I'd be happy to drop part of these changes and/or consider an alternative interface for exec.

Currently the isort provider uses exec and I'd hoped for a minimally invasive change by using the SpawnOptions to pass the input.

I had hoped that it would be generally useful to be able to pass arbitrary input to an exec and as that's the method used by the isort provider currently I think it would make keep that being the case.

An alternative change I had considered (but decided against) would be to have exec accept another (optional) parameter of input: string. Would that (and undoing the change to execObservable) be preferable? (This would allow the isort provide to keep using exec, which is simpler, while providing a generally useful enhancement)

[PeterJCLaw]

(meta) @karrtikr thanks for your help so far, I'll pick up any other changes tomorrow evening (I'm in GMT).

[DonJayamanne]

Still not a fan of input.
I leave this for @karthiknadig. @pvscteam
Feels this is an exceptional case just for one usage, which could be done by accessing the process itself.

[PeterJCLaw]

Just had a thought about this while writing the docs for it -- should we have this do an explicit null or undefined check, rather than this implicit one? I'm assuming that (like in Python) this implicit check will be falsey for both null and empty string, yet the latter might end up being surprising to callers if they've explicitly provided a value which happens to be empty.

[karthiknadig]

@PeterJCLaw we need to discus this with the team. Based on the decision we will let you know if we want to take this or go with @DonJayamanne 's suggestions.

[PeterJCLaw]

@karthiknadig just wondering if there's an update on this?

[karthiknadig]

@PeterJCLaw Sorry, we haven't gotten to this yet. We will have a discussion on this item this sprint.

[PeterJCLaw]

@karthiknadig I updated this for master recently, but it looks like it's still failing in the same way (that I don't really understand).

Any chance you'll be able to look at this soon?

[karthiknadig]

@PeterJCLaw We are fixing some flaky tests. We will get to this one. I will take care of addressing this.

[karthiknadig]

@PeterJCLaw Looks like isort does not pick up .isort.cfg when using stdin.
https://github.com/timothycrosley/isort/blob/5de8d50075f1dbe33a7e74172a6b6c27795a0c8d/isort/main.py#L596

C:\GIT\issues\s p\vscode-python\src\test\pythonFiles\sorting\withconfig>type "C:\GIT\issues\s p\vscode-python\src\test\pythonFiles\sorting\withconfig\original.py" | "C:\GIT\issues\s p\vscode-python\pythonFiles\lib\python\bin\isort" - --diff

If I pass the full path it seems to work as expected.

C:\GIT\issues\s p\vscode-python\src\test\pythonFiles\sorting\withconfig>"C:\GIT\issues\s p\vscode-python\pythonFiles\lib\python\bin\isort" original.py --diff
--- C:\GIT\issues\s p\vscode-python\src\test\pythonFiles\sorting\withconfig\original.py:before  2019-07-25 18:23:40.425993
+++ C:\GIT\issues\s p\vscode-python\src\test\pythonFiles\sorting\withconfig\original.py:after   2020-05-04 22:34:59.942799
@@ -1,3 +1,9 @@
-from third_party import (lib1, lib2, lib3,
-                         lib4, lib5, lib6,
-                         lib7, lib8, lib9)+from third_party import lib1
+from third_party import lib2
+from third_party import lib3
+from third_party import lib4
+from third_party import lib5
+from third_party import lib6
+from third_party import lib7
+from third_party import lib8
+from third_party import lib9

This is causing the test to fail. This should probably be filed as a bug on isort. This is a blocker to accept this change.

[PeterJCLaw]

Thanks for getting back to me.

@PeterJCLaw Looks like isort does not pick up .isort.cfg when using stdin.
https://github.com/timothycrosley/isort/blob/5de8d50075f1dbe33a7e74172a6b6c27795a0c8d/isort/main.py#L596

Hrm, that's rather odd. What versions of Python and isort were you testing with? (And what version is the CI picking up?)

I can reproduce that issue (on Linux too) when using the latest development version of isort from GitHub, however using latest published version on PyPI (a fresh pip install isort on Python 3.6 on windows 10 gets version 4.3.21) I'm unable to reproduce this.

The logs do seem to indicate that CI is picking up 4.3.21 though, so I'm not sure that this is the issue.

This should probably be filed as a bug on isort.

There are actually a couple of issues with the latest isort development branch which look like they'll affect us -- I've raised them as PyCQA/isort#1188 and PyCQA/isort#1189.

---

Note that isort appears to now be developing a 5.x branch, rather than the current 4.3.x branch. That has dropped support for running on Python versions before 3.6 (though claims to still be able to parse & fix older versions) so I'm guessing there could be some more complications in the near future with that branch,

[karthiknadig]

On windows 10 in am using 4.3.21. CI is using the same version, since it uses requirements.txt that is pinned to that version.

[PeterJCLaw]

Ah, I figured out why I couldn't reproduce this. I'm not sure what this means yet though.

For me, this works:

>isort - --diff < demo.py
--- :before     2020-05-06 00:09:28.178022
+++ :after      2020-05-06 00:09:28.178022
@@ -1 +1,3 @@
-import os, sys, collections
+import os
+import sys
+import collections

>

But this doesn't:

>type demo.py | isort - --diff

> 

I get a different diff if I change the config, so I think the config is a red herring here. I think the issue is going to be something around how pipes are handled.

I can also reproduce this on both Python 3.6.8 and 3.8.2 (on Windows 10). When I'm back at my other machine I'll try the pipe spelling on Linux, I wonder if it breaks there too? (I've only ever tried the < spelling there)

[karthiknadig]

The test is failing on CI because it is not picking up .isort.cfg. The diff comes back empty, when it should be applying force_single_line=True. I see that the cwd when spawning the process is set to the directory where the file is present. I don't see anything wrong there. I wonder why isort does not see the the config file.

[karthiknadig]

@PeterJCLaw If we are always using diff and we are going to send file content over stdin in all cases, then may be we should update sortImports.py to use isort.SortImports instead of going through main. That API seems to do the right thing.

[PeterJCLaw]

Ok, after some more digging, I don't think this relates to the configuration file at all. If I debug through isort in the failing case, it finds the configuration file just fine, but fails to read in the content of the python file after determining the encoding to use.

I think there are a couple of issues here:

• isort tries to seek within the incoming file, yet seeking within stdin isn't always possible in general and in Python doesn't seem to be as buffered might be assumed
• I think there might be a Python bug where if you read of the end of a buffered stream and then try to .seek(0) you don't get back to the start, so even adding our own buffering wrapping (via io.BufferedReader(...)) doesn't solve this for short files, such as the one in this test

Notably, adding a few comment lines to the test file makes isort work for me.

I've reproduced these outside isort while being specific to Windows and small files being sent to Python via stdin. I think there might be a Python bug here, which I've reported as https://bugs.python.org/issue40540.

Switching to using isort.SortImports directly can mitigate this as long as we pass io.BufferedReader(sys.stdin.buffer) as the file, though my testing shows that even that may not be sufficient for small files.
This might also have other impacts as the reader would then be an IO[str] rather than an IO[bytes], which we would need to be sure didn't have any adverse effects.

[karthiknadig]

For isort.SortImports we can pass the file contents directly using file_contents or dump the entire stdin into a stringIO and pass that as the file.

isort.SortImports(file_contents=text, show_diff=True)

[PeterJCLaw]

For isort.SortImports we can pass the file contents directly using file_contents or dump the entire stdin into a stringIO and pass that as the file.

isort.SortImports(file_contents=text, show_diff=True)

Yup, that sounds reasonable. I'll look at making that change this evening or tomorrow.

[PeterJCLaw]

For isort.SortImports we can pass the file contents directly using file_contents or dump the entire stdin into a stringIO and pass that as the file.

isort.SortImports(file_contents=text, show_diff=True)

Yup, that sounds reasonable. I'll look at making that change this evening or tomorrow.

It's occurred to me that this would actually mean we'd either lose support for isort command line arguments or need to parse them (albeit using the isort functions) ourselves.

I'm currently exploring replacing sys.stdin with an io.BytesIO, which I think will also work but avoid that issue.

[sonarcloud]

SonarCloud Quality Gate failed.

5 Bugs
0 Vulnerabilities (and 8 Security Hotspots to review)
30 Code Smells

No Coverage information
1.2% Duplication

[karrtikr]

LGTM. We can merge this as soon as CI passes. Thanks for working on this, and the patience @PeterJCLaw 🙂

Addressed

[karrtikr]

Users expect the cwd to point to the workspace root instead: #14254.