Add regex to dedent else and friends (#6497)

* Dedent on elif|else
* Add unit tests (will conflict with #4241)
* More descriptive comment (same as f5daf78)
* Move tests from extension to language config file
* Add news file
* Use regex we just introduced
* Space is free
* Update regexes and existing tests (so they pass)
* Add support for open tuples and dictionaries
* Shuffle and add tests
* Typo + more descriptive key name
* Updated news file after new changes
* Apply suggestions from code review (first batch)
Co-Authored-By: Eric Snow <[email protected]>
* Update src/client/language/languageConfiguration.ts
Co-Authored-By: Eric Snow <[email protected]>
* Third batch of review fixes
* Add some comments
* Separate unit tests
* More test simplification
* Simplify return regex
* Remove unnecessary tslint disable rule
* pipeline is stuck

Author: kimadeline

news/2 Fixes/6333.md

@@ -0,0 +1 @@
+Clarify regexes used for decreasing indentation.

src/client/extension.ts

@@ -40,7 +40,7 @@ import { registerTypes as appRegisterTypes } from './application/serviceRegistry
 import { IApplicationDiagnostics } from './application/types';
 import { DebugService } from './common/application/debugService';
 import { IApplicationShell, ICommandManager, IWorkspaceService } from './common/application/types';
-import { Commands, isTestExecution, PYTHON, STANDARD_OUTPUT_CHANNEL } from './common/constants';
+import { Commands, isTestExecution, PYTHON, PYTHON_LANGUAGE, STANDARD_OUTPUT_CHANNEL } from './common/constants';
 import { registerTypes as registerDotNetTypes } from './common/dotnet/serviceRegistry';
 import { registerTypes as installerRegisterTypes } from './common/installer/serviceRegistry';
 import { traceError } from './common/logger';
@@ -85,7 +85,7 @@ import { registerTypes as interpretersRegisterTypes } from './interpreter/servic
 import { ServiceContainer } from './ioc/container';
 import { ServiceManager } from './ioc/serviceManager';
 import { IServiceContainer, IServiceManager } from './ioc/types';
-import { setLanguageConfiguration } from './language/languageConfiguration';
+import { getLanguageConfiguration } from './language/languageConfiguration';
 import { LinterCommands } from './linters/linterCommands';
 import { registerTypes as lintersRegisterTypes } from './linters/serviceRegistry';
 import { ILintingEngine } from './linters/types';
@@ -173,7 +173,7 @@ async function activateUnsafe(context: ExtensionContext): Promise<IExtensionApi>
     const linterProvider = new LinterProvider(context, serviceManager);
     context.subscriptions.push(linterProvider);
 
-    setLanguageConfiguration();
+    languages.setLanguageConfiguration(PYTHON_LANGUAGE, getLanguageConfiguration());
 
     if (pythonSettings && pythonSettings.formatting && pythonSettings.formatting.provider !== 'internalConsole') {
         const formatProvider = new PythonFormattingEditProvider(context, serviceContainer);

src/client/language/languageConfiguration.ts

@@ -2,19 +2,24 @@
 // Licensed under the MIT License.
 'use strict';
 
-import { IndentAction, languages } from 'vscode';
-import { PYTHON_LANGUAGE } from '../common/constants';
+import { IndentAction } from 'vscode';
 
 export const MULTILINE_SEPARATOR_INDENT_REGEX = /^(?!\s+\\)[^#\n]+\\$/;
+/**
+ * This does not handle all cases. However, it does handle nearly all usage.
+ * Here's what it does not cover:
+ * - the statement is split over multiple lines (and hence the ":" is on a different line)
+ * - the code block is inlined (after the ":")
+ * - there are multiple statements on the line (separated by semicolons)
+ * Also note that `lambda` is purposefully excluded.
+ */
+export const INCREASE_INDENT_REGEX = /^\s*(?:(?:async|class|def|elif|except|for|if|while|with)\b.*|(else|finally|try))\s*:\s*(#.*)?$/;
+export const DECREASE_INDENT_REGEX = /^\s*(?:else|finally|(?:elif|except)\b.*)\s*:\s*(#.*)?$/;
+export const OUTDENT_ONENTER_REGEX = /^\s*(?:break|continue|pass|(?:raise|return)\b.*)\s*(#.*)?$/;
 
-export function setLanguageConfiguration() {
-    // Enable indentAction
-    languages.setLanguageConfiguration(PYTHON_LANGUAGE, {
+export function getLanguageConfiguration() {
+    return {
         onEnterRules: [
-            {
-                beforeText: /^\s*(?:def|class|for|if|elif|else|while|try|with|finally|except|async)\b.*:\s*/,
-                action: { indentAction: IndentAction.Indent }
-            },
             {
                 beforeText: MULTILINE_SEPARATOR_INDENT_REGEX,
                 action: { indentAction: IndentAction.Indent }
@@ -25,10 +30,13 @@ export function setLanguageConfiguration() {
                 action: { indentAction: IndentAction.None, appendText: '# ' }
             },
             {
-                beforeText: /^\s+(continue|break|return)\b.*/,
-                afterText: /\s+$/,
+                beforeText: OUTDENT_ONENTER_REGEX,
                 action: { indentAction: IndentAction.Outdent }
             }
-        ]
-    });
+        ],
+        indentationRules: {
+            increaseIndentPattern: INCREASE_INDENT_REGEX,
+            decreaseIndentPattern: DECREASE_INDENT_REGEX
+        }
+    };
 }

src/test/language/languageConfiguration.unit.test.ts

@@ -5,19 +5,84 @@
 
 import { expect } from 'chai';
 
-import { MULTILINE_SEPARATOR_INDENT_REGEX } from '../../client/language/languageConfiguration';
+import { DECREASE_INDENT_REGEX, INCREASE_INDENT_REGEX, MULTILINE_SEPARATOR_INDENT_REGEX, OUTDENT_ONENTER_REGEX } from '../../client/language/languageConfiguration';
 
 suite('Language configuration regexes', () => {
     test('Multiline separator indent regex should not pick up strings with no multiline separator', async () => {
         const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = "test"');
-        expect (result).to.be.equal(false, 'Multiline separator indent regex for regular strings should not have matches');
+        expect(result).to.be.equal(false, 'Multiline separator indent regex for regular strings should not have matches');
     });
+
     test('Multiline separator indent regex should not pick up strings with escaped characters', async () => {
         const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = \'hello \\n\'');
-        expect (result).to.be.equal(false, 'Multiline separator indent regex for strings with escaped characters should not have matches');
+        expect(result).to.be.equal(false, 'Multiline separator indent regex for strings with escaped characters should not have matches');
     });
+
     test('Multiline separator indent regex should pick up strings ending with a multiline separator', async () => {
         const result = MULTILINE_SEPARATOR_INDENT_REGEX.test('a = \'multiline \\');
-        expect (result).to.be.equal(true, 'Multiline separator indent regex for strings with newline separator should have matches');
+        expect(result).to.be.equal(true, 'Multiline separator indent regex for strings with newline separator should have matches');
+    });
+
+    [
+        'async def test(self):',
+        'class TestClass:',
+        'def foo(self, node, namespace=""):',
+        'for item in items:',
+        'if foo is None:',
+        'try:',
+        'while \'::\' in macaddress:',
+        'with self.test:'
+    ].forEach(example => {
+        const keyword = example.split(' ')[0];
+
+        test(`Increase indent regex should pick up lines containing the ${keyword} keyword`, async () => {
+            const result = INCREASE_INDENT_REGEX.test(example);
+            expect(result).to.be.equal(true, `Increase indent regex should pick up lines containing the ${keyword} keyword`);
+        });
+
+        test(`Decrease indent regex should not pick up lines containing the ${keyword} keyword`, async () => {
+            const result = DECREASE_INDENT_REGEX.test(example);
+            expect(result).to.be.equal(false, `Decrease indent regex should not pick up lines containing the ${keyword} keyword`);
+        });
+    });
+
+    ['elif x < 5:', 'else:', 'except TestError:', 'finally:'].forEach(example => {
+        const keyword = example.split(' ')[0];
+
+        test(`Increase indent regex should pick up lines containing the ${keyword} keyword`, async () => {
+            const result = INCREASE_INDENT_REGEX.test(example);
+            expect(result).to.be.equal(true, `Increase indent regex should pick up lines containing the ${keyword} keyword`);
+        });
+
+        test(`Decrease indent regex should pick up lines containing the ${keyword} keyword`, async () => {
+            const result = DECREASE_INDENT_REGEX.test(example);
+            expect(result).to.be.equal(true, `Decrease indent regex should pick up lines containing the ${keyword} keyword`);
+        });
+    });
+
+    test('Increase indent regex should not pick up lines without keywords', async () => {
+        const result = INCREASE_INDENT_REGEX.test('a = \'hello \\n \'');
+        expect(result).to.be.equal(false, 'Increase indent regex should not pick up lines without keywords');
+    });
+
+    test('Decrease indent regex should not pick up lines without keywords', async () => {
+        const result = DECREASE_INDENT_REGEX.test('a = \'hello \\n \'');
+        expect(result).to.be.equal(false, 'Decrease indent regex should not pick up lines without keywords');
+    });
+
+    ['    break', '\t\t continue', ' pass', 'raise Exception(\'Unknown Exception\'', '    return [ True, False, False ]'].forEach(example => {
+        const keyword = example.trim().split(' ')[0];
+
+        const testWithoutComments = `Outdent regex for on enter rule should pick up lines containing the ${keyword} keyword`;
+        test(testWithoutComments, () => {
+            const result = OUTDENT_ONENTER_REGEX.test(example);
+            expect(result).to.be.equal(true, testWithoutComments);
+        });
+
+        const testWithComments = `Outdent regex on enter should pick up lines containing the ${keyword} keyword and ending with comments`;
+        test(testWithComments, () => {
+            const result = OUTDENT_ONENTER_REGEX.test(`${example} # test comment`);
+            expect(result).to.be.equal(true, testWithComments);
+        });
     });
 });