Implements linting configuration by MikhailArkhipov · Pull Request #599 · microsoft/vscode-python

MikhailArkhipov · 2018-01-18T23:27:58Z

Fixes #572

Tests pass, merger with @DonJayamanne TS changes.

Implemented 'linter info' for per-linter information
Implemented linter manager for various linter-related operations
Implemented linter commands
Added 'python settings provider' service in order to avoid referencing static classes
Split big 'installer' class into multiple for linters, formatters, ctags, frameworks
New tests
Fixes bunch of missing async/awaits

…m list

codecov · 2018-01-19T18:01:47Z

Codecov Report

Merging #599 into master will decrease coverage by 0.06%.
The diff coverage is 78.67%.

@@            Coverage Diff            @@
##           master    #599      +/-   ##
=========================================
- Coverage   60.67%   60.6%   -0.07%     
=========================================
  Files         228     231       +3     
  Lines       10125   10478     +353     
  Branches     1753    1827      +74     
=========================================
+ Hits         6143    6350     +207     
- Misses       3977    4122     +145     
- Partials        5       6       +1

Impacted Files	Coverage Δ
src/client/common/types.ts	`100% <ø> (ø)`	⬆️
...rc/client/common/errors/moduleNotInstalledError.ts	`33.33% <0%> (ø)`	⬆️
src/client/linters/types.ts	`100% <100%> (ø)`	⬆️
src/client/common/installer/pipInstaller.ts	`87.5% <100%> (+17.12%)`	⬆️
src/client/formatters/autoPep8Formatter.ts	`95% <100%> (ø)`	⬆️
...c/client/linters/errorHandlers/baseErrorHandler.ts	`77.77% <100%> (+2.77%)`	⬆️
...rc/client/common/process/pythonExecutionFactory.ts	`100% <100%> (ø)`	⬆️
src/client/common/serviceRegistry.ts	`100% <100%> (ø)`	⬆️
src/client/linters/pep8.ts	`100% <100%> (ø)`
src/client/extension.ts	`93.95% <100%> (ø)`	⬆️
... and 30 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 0253995...36e2fa9. Read the comment docs.

DonJayamanne · 2018-01-19T03:30:12Z

.vscode/settings.json

    "python.workspaceSymbols.enabled": false,
    "python.formatting.provider": "none",
-    "files.insertFinalNewline": true
+    "files.insertFinalNewline": true,


DonJayamanne · 2018-01-19T03:31:43Z

package.nls.json

    "python.command.jupyter.gotToNextCell.title": "Go to Next Cell",
    "python.command.python.goToPythonObject.title": "Go to Python Object",
+    "python.command.python.setLinter.title": "Select Linter",
+    "python.command.python.enableLinting.title": "Enable Linting",


Can we use this command to disable linting as well?
Three buttons, last being the default VS Code button.
E.g. Enable or disable linting [Enable Linting] [Disable Linting] [Close]

It currently shows on/off list

DonJayamanne · 2018-01-19T03:34:44Z

src/client/common/configSettings.ts

+export interface IPythonSettingsProvider {
+    getInstance(resource?: Uri): IPythonSettings;
+    // tslint:disable-next-line:no-any
+    updateSettingAsync(setting: string, value: any, resource?: Uri): Promise<void>;


Please change setting to a strongly typed value, we need to ensure only valid items are being set in here.
Similar to what has been done here https://github.com/Microsoft/vscode-python/blob/master/src/test/common.ts#L20

We also need a configuration target for this. Are we setting in:

Global

Workspace

Workspace folder

Again, please check https://github.com/Microsoft/vscode-python/blob/master/src/test/common.ts#L20.

DonJayamanne · 2018-01-19T03:38:36Z

src/client/extension.ts

-    const deprecationMgr = new FeatureDeprecationManager(persistentStateFactory, !!jupyterExtInstalled);
-    deprecationMgr.initialize();
-    context.subscriptions.push(new FeatureDeprecationManager(persistentStateFactory, !!jupyterExtInstalled));
+function configureIndentActions(): void {


Question: Should we move this into a separate folder, languages or formatters?

DonJayamanne · 2018-01-19T03:41:01Z

src/client/linters/linterInfo.ts

+        return settings.linting[this.enabledSettingName] as boolean;
+    }
+
+    public pathName(resource?: Uri): string {


should this be named getPathName as its a method (verb)

DonJayamanne · 2018-01-19T03:41:15Z

src/client/linters/linterInfo.ts

+        const settings = this.settingsProvider.getInstance(resource);
+        return settings.linting[this.pathSettingName] as string;
+    }
+    public linterArgs(resource?: Uri): string[] {


should this be named getLinterArgs as its a method (verb)

DonJayamanne · 2018-01-19T03:48:15Z

src/client/linters/linterCommands.ts

+    constructor(private serviceContainer: IServiceContainer, registerCommands: boolean) {
+        this.linterManager = this.serviceContainer.get<ILinterManager>(ILinterManager);
+        this.appShell = this.serviceContainer.get<IApplicationShell>(IApplicationShell);
+        if (registerCommands) {


FYI - No need to change, but I've introduced a ICommandManager to wrap the vscode.scommands namespace

Changed to CM

DonJayamanne · 2018-01-21T19:48:54Z

src/client/common/configuration/service.ts

+
+    private async verifySetting(pythonConfig: WorkspaceConfiguration, target: ConfigurationTarget, settingName: string, value: {}): Promise<void> {
+        if (isTestExecution()) {
+            let retries = 0;


Can't we mock this? I have started moving this settings with the creation of workspacemanager to wrap vscode.wor

This basically for test stability. It guarantees that the setting has actually updated (rather than waiting 5 seconds as in a few other places)

Agreed, but I'm still reluctant to have this test specific code in the (production code) extension. Can't we move this into just the tests?

DonJayamanne · 2018-01-21T19:53:56Z

src/client/common/configuration/service.ts

+
+        await pythonConfig.update(setting, value, settingsInfo.target);
+        await this.verifySetting(pythonConfig, settingsInfo.target, setting, value);
+        PythonSettings.dispose();


Do we need the check verify setting, I am reluctant to have it just for our tests. This should always work in production, hence not necessary, and mocking it should work.

DonJayamanne · 2018-01-21T23:12:32Z

package.nls.json

    "python.command.jupyter.gotToNextCell.title": "Go to Next Cell",
    "python.command.python.goToPythonObject.title": "Go to Python Object",
+    "python.command.python.setLinter.title": "Select Linter",
+    "python.command.python.enableLinting.title": "Enable Linting",


DonJayamanne · 2018-01-21T23:14:06Z

src/client/common/configSettings.ts

    }
+
+    public static getSettingsUriAndTarget(resource?: Uri): { uri: Uri | undefined, target: ConfigurationTarget } {
+        const workspaceFolder = resource ? vscode.workspace.getWorkspaceFolder(resource) : undefined;


Please use IWorkspaceService instead of the vscode.workspace namespace. You should be able to mock the settings this way and be able to remove test specific code from the extension as well.

Actually, I prefer to use actual objects where possible since they work same way in tests as they do at run time. Mocking is more for something that cannot be instantiated easily for whatever reason (ex project system in big VS) or simulate things that are hard to change (such as registry data - which requires Windows and admin rights).

DonJayamanne · 2018-01-21T23:15:26Z

src/client/common/configuration/service.ts

+
+    private async verifySetting(pythonConfig: WorkspaceConfiguration, target: ConfigurationTarget, settingName: string, value?: {}): Promise<void> {
+        if (isTestExecution()) {
+            let retries = 0;


Can we get rid of this, and use the mocked vscode.workspace namespace.
I'm very reluctant to have test specific code in here.

What I've done in such situations is avoided updating settings, and just using a specific workspace with hardcoded settings in the settings.json file (if you look at the testMultiRootWkspace folder, you can see multiple workspace folders in there, some have settings.json with specific settings in there.

It works at runtime, just not instantly. I observed actual value change in ~500-1000ms (and the config change event to fire). In tests it often ends up reading value that is stale. So it is not necessary to delay action in production but in tests it improves stability.

So it is not necessary to delay action in production but in tests it improves stability.

If thats the case, why not move this delay into the tests itself. As this block of code never gets used in production, i.e. its test specific?

My only concern about this is the proliferation of if isTetExecution() in our code, just for tests.

The problem here is that test would need to know that code it is calling might update the settings. For example, LinterManager.enableLinting and friends - and to know what key to use for ensuring the updated value. Since updating setting is rarely performance-critical (if ever), we can simple get it called in all cases. I'd reduce step to, say, 200ms and have 25 retries. It most cases operation completes in ~500ms so it should not be a big deal anyway.

DonJayamanne · 2018-01-21T23:23:30Z

src/client/terminals/codeExecution/terminalCodeExecution.ts

        const pythonSettings = this.configurationService.getSettings(file);

-        this.setCwdForFileExecution(file);
+        await this.setCwdForFileExecution(file);


…nto lint Fix Windows terminal tests

… into lint

MikhailArkhipov added 30 commits December 1, 2017 10:18

Basic tokenizer

7675901

Fixed property names

eb42669

Tests, round I

2756974

Tests, round II

c2c1ced

merge master

a108c96

tokenizer test

14864a5

Remove temorary change

0ed51d6

Fix merge issue

51b544c

Merge conflict

3cd11e6

Merge conflict

82e0ad1

Completion test

9295c1a

Fix last line

06eb1a5

Fix javascript math

e9db8e0

Merge master

d12ca03

Make test await for results

d8ab041

Add license headers

db75cd0

Rename definitions to types

9ab2c47

License headers

d587485

Merge branch 'master' of https://github.com/Microsoft/vscode-python

1da5e0a

Merge branch 'master' of https://github.com/Microsoft/vscode-python

7668cee

Fix typo in completion details (typo)

1ac4932

Fix hover test

2aa5a6c

Merge branch 'master' of https://github.com/Microsoft/vscode-python

5db31bd

Russian translations

560d2af

Merge branch 'master' of https://github.com/Microsoft/vscode-python

c71024d

Update to better translation

31aa087

Fix typo

593ae05

#70 How to get all parameter info when filling in a function param list

e6d69bb

Fix #70 How to get all parameter info when filling in a function para…

b5a23d3

…m list

Clean up

cd200f7

MikhailArkhipov added 5 commits January 19, 2018 10:29

Test fixes

2f361f5

Fix terminal service and tests async/await

384ca3a

Fix mock setup

4dc23fb

Test fix

82e4d52

Test async/await fix

8207953

DonJayamanne requested changes Jan 19, 2018

View reviewed changes

MikhailArkhipov added 8 commits January 19, 2018 14:47

Test fix + activate tslint on awaits

8b701d2

Use command manager

c5ece0a

Work around updateSettings

5972c11

Multiroot fixes, partial

11a4891

More workarounds

84562bf

Multiroot tests

b5c547a

Fix installer test

ecb90a6

Test fixes

cd79d03

DonJayamanne reviewed Jan 21, 2018

View reviewed changes

MikhailArkhipov added 5 commits January 22, 2018 10:11

Disable prospector

27a4244

Enable dispose in all cases

34365a5

Fix event firing

9071ea8

Min pylint options

4e2c620

Min checkers & pylintrc discovery

c090581

DonJayamanne approved these changes Jan 23, 2018

View reviewed changes

MikhailArkhipov added 5 commits January 23, 2018 15:23

Fix Windows path in tests for Travis

96fcba1

Fix Mac test

25c7d0a

Merge branch 'master' of https://github.com/Microsoft/vscode-python i…

c824c3e

…nto lint Fix Windows terminal tests

Merge branch 'lint' of https://github.com/MikhailArkhipov/vscode-python…

9de1d81

… into lint

Test fix

36e2fa9

MikhailArkhipov merged commit df39217 into microsoft:master Jan 24, 2018

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

Conversation

MikhailArkhipov commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

MikhailArkhipov Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MikhailArkhipov commented Jan 18, 2018 •

edited

Loading

codecov bot commented Jan 19, 2018 •

edited

Loading

MikhailArkhipov Jan 22, 2018 •

edited

Loading