Hot fixes for pylint and jediEnabled issues (#12556)

* Fix `linting.pylintEnabled` setting check (#12444)

* Fix `linting.pylintEnabled` setting check
* Use stub instead of handspun variable

* Don't modify LS settings if jediEnabled does not exist (#12551)

* Update change logs and version

Co-authored-by: Kim-Adeline Miguel <[email protected]>
Co-authored-by: Jake Bailey <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+## 2020.6.2 (25 June 2020)
+
+### Fixes
+
+1. Fix `linting.pylintEnabled` setting check.
+   ([#12285](https://github.com/Microsoft/vscode-python/issues/12285))
+1. Don't modify LS settings if jediEnabled does not exist.
+   ([#12429](https://github.com/Microsoft/vscode-python/issues/12429))
+
 ## 2020.6.1 (17 June 2020)
 
 ### Fixes

package-lock.json

@@ -2,7 +2,7 @@
     "name": "python",
     "displayName": "Python",
     "description": "Linting, Debugging (multi-threaded, remote), Intellisense, Jupyter Notebooks, code formatting, refactoring, unit tests, snippets, and more.",
-    "version": "2020.6.1",
+    "version": "2020.6.2",
     "featureFlags": {
         "usingNewInterpreterStorage": true
     },

package.json

@@ -99,7 +99,7 @@ export class PylintLinterInfo extends LinterInfo {
         // If we're using LS, then by default Pylint is disabled unless user provided
         // the value. We have to resort to direct inspection of settings here.
         const configuration = this.workspaceService.getConfiguration('python', resource);
-        const inspection = configuration.inspect<boolean>(this.enabledSettingName);
+        const inspection = configuration.inspect<boolean>(`linting.${this.enabledSettingName}`);
         if (
             !inspection ||
             (inspection.globalValue === undefined &&

src/client/linters/linterInfo.ts

@@ -4,7 +4,7 @@
 'use strict';
 
 import { inject, injectable } from 'inversify';
-import { applyEdits, Edit, findNodeAtLocation, FormattingOptions, getNodeValue, modify, parseTree } from 'jsonc-parser';
+import { applyEdits, findNodeAtLocation, getNodeValue, ModificationOptions, modify, parseTree } from 'jsonc-parser';
 import * as path from 'path';
 import { IExtensionActivationService, LanguageServerType } from '../../activation/types';
 import { IApplicationEnvironment, IWorkspaceService } from '../../common/application/types';
@@ -104,7 +104,8 @@ export class UpdateTestSettingService implements IExtensionActivationService {
 
     private fixLanguageServerSettings(fileContent: string): string {
         // `python.jediEnabled` is deprecated:
-        //   - `true` or missing then set to `languageServer: Jedi`.
+        //   - If missing, do nothing.
+        //   - `true`, then set to `languageServer: Jedi`.
         //   - `false` and `languageServer` is present, do nothing.
         //   - `false` and `languageServer` is NOT present, set `languageServer` to `Microsoft`.
         // `jediEnabled` is NOT removed since JSONC parser may also remove comments.
@@ -113,28 +114,40 @@ export class UpdateTestSettingService implements IExtensionActivationService {
 
         try {
             const ast = parseTree(fileContent);
+
             const jediEnabledNode = findNodeAtLocation(ast, jediEnabledPath);
-            const jediEnabled = jediEnabledNode ? getNodeValue(jediEnabledNode) : true;
             const languageServerNode = findNodeAtLocation(ast, languageServerPath);
-            const formattingOptions: FormattingOptions = {
-                tabSize: 4,
-                insertSpaces: true
-            };
-            let edits: Edit[] = [];
-
-            if (!jediEnabledNode || jediEnabled) {
-                // `jediEnabled` is missing or is true. Default is true, so assume Jedi.
-                edits = modify(fileContent, languageServerPath, LanguageServerType.Jedi, { formattingOptions });
-            } else {
-                // `jediEnabled` is false. if languageServer is missing, set it to Microsoft.
-                if (!languageServerNode) {
-                    edits = modify(fileContent, languageServerPath, LanguageServerType.Microsoft, {
-                        formattingOptions
-                    });
+
+            // If missing, do nothing.
+            if (!jediEnabledNode) {
+                return fileContent;
+            }
+
+            const jediEnabled = getNodeValue(jediEnabledNode);
+
+            const modificationOptions: ModificationOptions = {
+                formattingOptions: {
+                    tabSize: 4,
+                    insertSpaces: true
                 }
+            };
+
+            // `jediEnabled` is true, set it to Jedi.
+            if (jediEnabled) {
+                return applyEdits(
+                    fileContent,
+                    modify(fileContent, languageServerPath, LanguageServerType.Jedi, modificationOptions)
+                );
+            }
+
+            // `jediEnabled` is false. if languageServer is missing, set it to Microsoft.
+            if (!languageServerNode) {
+                return applyEdits(
+                    fileContent,
+                    modify(fileContent, languageServerPath, LanguageServerType.Microsoft, modificationOptions)
+                );
             }
 
-            fileContent = applyEdits(fileContent, edits);
             // tslint:disable-next-line:no-empty
         } catch {}
         return fileContent;

src/client/testing/common/updateTestSettings.ts

@@ -217,9 +217,14 @@ suite('Application Diagnostics - Check Test Settings', () => {
 
     [
         {
-            testTitle: 'No jediEnabled setting.',
+            testTitle: 'No jediEnabled setting',
             contents: '{}',
-            expectedContent: '{ "python.languageServer": "Jedi" }'
+            expectedContent: '{}'
+        },
+        {
+            testTitle: 'jediEnabled setting in comment',
+            contents: '{\n // "python.jediEnabled": true\n }',
+            expectedContent: '{\n // "python.jediEnabled": true\n }'
         },
         {
             testTitle: 'jediEnabled: true, no languageServer setting',
@@ -250,6 +255,11 @@ suite('Application Diagnostics - Check Test Settings', () => {
             testTitle: 'jediEnabled: false, languageServer is Jedi',
             contents: '{ "python.jediEnabled": false, "python.languageServer": "Jedi" }',
             expectedContent: '{ "python.jediEnabled": false, "python.languageServer": "Jedi"}'
+        },
+        {
+            testTitle: 'jediEnabled not present, languageServer is Microsoft',
+            contents: '{ "python.languageServer": "Microsoft" }',
+            expectedContent: '{ "python.languageServer": "Microsoft" }'
         }
     ].forEach((item) => {
         test(item.testTitle, async () => {

src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts

@@ -6,6 +6,7 @@
 // tslint:disable:chai-vague-errors no-unused-expression max-func-body-length no-any
 
 import { expect } from 'chai';
+import * as sinon from 'sinon';
 import { anything, instance, mock, when } from 'ts-mockito';
 import { LanguageServerType } from '../../client/activation/types';
 import { WorkspaceService } from '../../client/common/application/workspace';
@@ -62,6 +63,22 @@ suite('Linter Info - Pylint', () => {
 
         expect(linterInfo.isEnabled()).to.be.false;
     });
+    test('Should inspect the value of linting.pylintEnabled when using Language Server', async () => {
+        const linterInfo = new PylintLinterInfo(instance(config), instance(workspace), []);
+        const inspectStub = sinon.stub();
+        const pythonConfig = {
+            inspect: inspectStub
+        };
+
+        when(config.getSettings(anything())).thenReturn({
+            linting: { pylintEnabled: true },
+            languageServer: LanguageServerType.Microsoft
+        } as any);
+        when(workspace.getConfiguration('python', anything())).thenReturn(pythonConfig as any);
+
+        expect(linterInfo.isEnabled()).to.be.false;
+        expect(inspectStub.calledOnceWith('linting.pylintEnabled')).to.be.true;
+    });
     const testsForisEnabled = [
         {
             testName: 'When workspaceFolder setting is provided',