Sort imports using URI and apply edits using workspace edits by DonJayamanne · Pull Request #2214 · microsoft/vscode-python

DonJayamanne · 2018-07-21T09:52:23Z

Pending manual testing.

Title summarizes what is changing
Includes a news entry file (remember to thank yourself!)
Unit tests & code coverage are not adversely affected (within reason)
Works on all actively maintained versions of Python (e.g. Python 2.7 & the latest Python 3 release)
Works on Windows 10, macOS, and Linux (e.g. considered file system case-sensitivity)
[n/a] Dependencies are pinned (e.g. "1.2.3", not "^1.2.3")
[n/a] package-lock.json has been regenerated if dependencies have changed

codecov · 2018-07-21T10:14:44Z

Codecov Report

Merging #2214 into master will decrease coverage by 4.04%.
The diff coverage is 89.51%.

@@            Coverage Diff            @@
##           master   #2214      +/-   ##
=========================================
- Coverage   79.84%   75.8%   -4.05%     
=========================================
  Files         310     310              
  Lines       14359   14407      +48     
  Branches     2547    2552       +5     
=========================================
- Hits        11465   10921     -544     
- Misses       2882    3475     +593     
+ Partials       12      11       -1

Flag	Coverage Δ
#MacOS	`74.06% <89.51%> (+0.1%)`	⬆️
#Windows	`74.08% <89.51%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
src/client/common/application/types.ts	`100% <ø> (ø)`	⬆️
src/client/common/platform/types.ts	`100% <ø> (ø)`	⬆️
src/client/common/types.ts	`100% <ø> (ø)`	⬆️
src/client/common/application/documentManager.ts	`61.29% <100%> (+5.73%)`	⬆️
src/client/providers/serviceRegistry.ts	`100% <100%> (ø)`
src/client/common/platform/fileSystem.ts	`72.09% <100%> (+0.66%)`	⬆️
src/client/common/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/extension.ts	`95.51% <100%> (+0.08%)`	⬆️
src/client/providers/types.ts	`100% <100%> (ø)`
src/client/common/editor.ts	`72.68% <84.84%> (+2.09%)`	⬆️
... and 47 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 6af8f99...487cfa7. Read the comment docs.

d3r3kk · 2018-07-24T06:04:33Z

Just needs a rebuild, the Mac agents were updated recently.

d3r3kk

This change is great, but I think there are some things to resolve:

CI is failing on helper tests - knock on effect from this change perhaps.
Fix / add logic surrounding trailing-line hack.

d3r3kk · 2018-08-01T19:08:54Z

 // We do this to ensure we only run debugger test, as debugger tests are very flaky on CI.
 // So the solution is to run them separately and first on CI.
-const grep = IS_CI_SERVER && IS_CI_SERVER_TEST_DEBUGGER ? 'Debug' : undefined;
+const grep = IS_CI_SERVER && IS_CI_SERVER_TEST_DEBUGGER ? 'Debug' : 'Sortingx';


Not sure we want this left in here...

d3r3kk · 2018-08-01T19:16:14Z

    debugConfigurationRegisterTypes(serviceManager);
    debuggerRegisterTypes(serviceManager);
    appRegisterTypes(serviceManager);
+    providersRegisterTypes(serviceManager);


I would expect to find all types of providers being registered here (formatProvider, signatureProvider... etc). Is that the intention here?

DI was added later. So we do not have others registered in the container.

That is the intention then, good to know.

d3r3kk · 2018-08-01T19:29:20Z

+            return;
+        }
+
+        // Hack, if the document doesn't contain an empty line at the end, then add it


Is this hack for the 'inner version' of isort only?

If so, I think we should skip this hack for when the settings.isortPath is set to somthing.

Bonus points if we could remove this hack by correcting our inner version of isort.

Is this hack for the 'inner version' of isort only?

No, we need an empty line for the diff to be genrated correctly.

Will get removed when we change the diffing coding, there's a separate PR to use an npm package for that.

Alright, I'll assume we'll rid ourselves of the hack when that #1950 issue is resolved.

d3r3kk · 2018-08-01T19:39:34Z

+            .setup(d => d.activeTextEditor).returns(() => undefined)
+            .verifiable(TypeMoq.Times.once());
+        shell
+            .setup(s => s.showErrorMessage(TypeMoq.It.isValue('Please open a Python file to sort the imports.')))


I think it would be important to define messages such as this in a common place (used by both the test and the feature itself). That way we can more be more complete and adept at supporting improvements our localization efforts.

No such functionality available now. Separate PR, we have an issue open for this. I'll try to complete it later.

Yep, I see it now: #463. I'm glad that we have an issue for it!

d3r3kk · 2018-08-01T19:40:12Z

+        if (!uri) {
+            const activeEditor = this.documentManager.activeTextEditor;
+            if (!activeEditor || activeEditor.document.languageId !== PYTHON_LANGUAGE) {
+                this.shell.showErrorMessage('Please open a Python file to sort the imports.').then(noop, noop);


Is there a localization file we can add this string to?

That will be done in a separate PR (bigger change).
There's a separate task for that.

d3r3kk · 2018-08-01T19:47:53Z

+        shell.verifyAll();
+        documentManager.verifyAll();
+    });
+    test('Ensure empty line is added when line does not end with an empty line', async () => {


This test may need some updating, see my comment on the subject of the trailing-line hack.

This will become unnecessary when we use an npm package to do the diffing. There's an issue for that.
#1950

DonJayamanne · 2018-08-02T22:52:49Z

@d3r3kk
Please review once again.

d3r3kk · 2018-08-02T23:21:58Z

Looks good, and for everything I raised we have separate issues tracking them. Now the build just needs to run clean (there are failing tests still).

brettcannon · 2018-08-20T17:23:26Z

@DonJayamanne anything holding up merging this?

DonJayamanne · 2018-09-06T20:41:10Z

Interestingly the tests were successful, however there were issues with the merge and compilation issues. Not sure how the tests ran successfully on CI!!!

created PR to resolve merge issues #2508

DonJayamanne closed this Jul 24, 2018

DonJayamanne reopened this Jul 24, 2018

d3r3kk self-requested a review August 1, 2018 18:53

d3r3kk self-assigned this Aug 1, 2018

d3r3kk suggested changes Aug 1, 2018

View reviewed changes

d3r3kk removed their assignment Aug 2, 2018

d3r3kk approved these changes Aug 2, 2018

View reviewed changes

DonJayamanne added 2 commits September 6, 2018 13:19

Merged master

f3d87e5

Fix code review comments

0fc9e95

DonJayamanne force-pushed the issue2137Actions branch from 487cfa7 to 0fc9e95 Compare September 6, 2018 20:22

DonJayamanne merged commit 7c646db into microsoft:master Sep 6, 2018

DonJayamanne deleted the issue2137Actions branch October 2, 2018 22:46

lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Conversation

DonJayamanne commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jul 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

d3r3kk commented Jul 24, 2018

Uh oh!

d3r3kk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne commented Aug 2, 2018

Uh oh!

d3r3kk commented Aug 2, 2018

Uh oh!

brettcannon commented Aug 20, 2018

Uh oh!

DonJayamanne commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DonJayamanne commented Jul 21, 2018 •

edited

Loading

codecov Bot commented Jul 21, 2018 •

edited

Loading

DonJayamanne commented Sep 6, 2018 •

edited

Loading