Refactor LinterId from string union to an enum (#12116)

* Refactor LinterId from string union to an enum and update tests and usages.

* Fix incorrect LINTERID_BY_PRODUCT entry and add news item

* Update news/3 Code Health/12116.md

Co-authored-by: Karthik Nadig <[email protected]>

* Revert lockfile changes

Co-authored-by: Karthik Nadig <[email protected]>

Author: tonybaloney

news/3 Code Health/12116.md

@@ -0,0 +1,2 @@
+Refactor `LinterId` to an enum instead of a string union.
+(thanks to [Anthony Shaw](https://github.com/tonybaloney))

src/client/linters/constants.ts

@@ -8,12 +8,12 @@ import { LinterId } from './types';
 
 // All supported linters must be in this map.
 export const LINTERID_BY_PRODUCT = new Map<Product, LinterId>([
-    [Product.bandit, 'bandit'],
-    [Product.flake8, 'flake8'],
-    [Product.pylint, 'pylint'],
-    [Product.mypy, 'mypy'],
-    [Product.pycodestyle, 'pycodestyle'],
-    [Product.prospector, 'prospector'],
-    [Product.pydocstyle, 'pydocstyle'],
-    [Product.pylama, 'pylama']
+    [Product.bandit, LinterId.Bandit],
+    [Product.flake8, LinterId.Flake8],
+    [Product.pylint, LinterId.PyLint],
+    [Product.mypy, LinterId.MyPy],
+    [Product.pycodestyle, LinterId.PyCodeStyle],
+    [Product.prospector, LinterId.Prospector],
+    [Product.pydocstyle, LinterId.PyDocStyle],
+    [Product.pylama, LinterId.PyLama]
 ]);

src/client/linters/linterInfo.ts

@@ -83,7 +83,7 @@ export class PylintLinterInfo extends LinterInfo {
         private readonly workspaceService: IWorkspaceService,
         configFileNames: string[] = []
     ) {
-        super(Product.pylint, 'pylint', configService, configFileNames);
+        super(Product.pylint, LinterId.PyLint, configService, configFileNames);
     }
     public isEnabled(resource?: Uri): boolean {
         // We want to be sure the setting is not default since default is `true` and hence

src/client/linters/linterManager.ts

@@ -18,12 +18,12 @@ import { Pycodestyle } from './pycodestyle';
 import { PyDocStyle } from './pydocstyle';
 import { PyLama } from './pylama';
 import { Pylint } from './pylint';
-import { IAvailableLinterActivator, ILinter, ILinterInfo, ILinterManager, ILintMessage } from './types';
+import { IAvailableLinterActivator, ILinter, ILinterInfo, ILinterManager, ILintMessage, LinterId } from './types';
 
 class DisabledLinter implements ILinter {
     constructor(private configService: IConfigurationService) {}
     public get info() {
-        return new LinterInfo(Product.pylint, 'pylint', this.configService);
+        return new LinterInfo(Product.pylint, LinterId.PyLint, this.configService);
     }
     public async lint(_document: TextDocument, _cancellation: CancellationToken): Promise<ILintMessage[]> {
         return [];
@@ -43,14 +43,14 @@ export class LinterManager implements ILinterManager {
         this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
         // Note that we use unit tests to ensure all the linters are here.
         this.linters = [
-            new LinterInfo(Product.bandit, 'bandit', this.configService),
-            new LinterInfo(Product.flake8, 'flake8', this.configService),
+            new LinterInfo(Product.bandit, LinterId.Bandit, this.configService),
+            new LinterInfo(Product.flake8, LinterId.Flake8, this.configService),
             new PylintLinterInfo(this.configService, this.workspaceService, ['.pylintrc', 'pylintrc']),
-            new LinterInfo(Product.mypy, 'mypy', this.configService),
-            new LinterInfo(Product.pycodestyle, 'pycodestyle', this.configService),
-            new LinterInfo(Product.prospector, 'prospector', this.configService),
-            new LinterInfo(Product.pydocstyle, 'pydocstyle', this.configService),
-            new LinterInfo(Product.pylama, 'pylama', this.configService)
+            new LinterInfo(Product.mypy, LinterId.MyPy, this.configService),
+            new LinterInfo(Product.pycodestyle, LinterId.PyCodeStyle, this.configService),
+            new LinterInfo(Product.prospector, LinterId.Prospector, this.configService),
+            new LinterInfo(Product.pydocstyle, LinterId.PyDocStyle, this.configService),
+            new LinterInfo(Product.pylama, LinterId.PyLama, this.configService)
         ];
     }
 

src/client/linters/types.ts

@@ -11,9 +11,16 @@ export interface IErrorHandler {
     handleError(error: Error, resource: vscode.Uri, execInfo: ExecutionInfo): Promise<boolean>;
 }
 
-// tslint:disable-next-line:no-suspicious-comment
-// TODO: Use an enum for LinterID instead of a union of string literals.
-export type LinterId = 'flake8' | 'mypy' | 'pycodestyle' | 'prospector' | 'pydocstyle' | 'pylama' | 'pylint' | 'bandit';
+export enum LinterId {
+    Flake8 = 'flake8',
+    MyPy = 'mypy',
+    PyCodeStyle = 'pycodestyle',
+    Prospector = 'prospector',
+    PyDocStyle = 'pydocstyle',
+    PyLama = 'pylama',
+    PyLint = 'pylint',
+    Bandit = 'bandit'
+}
 
 export interface ILinterInfo {
     readonly id: LinterId;

src/test/linters/lint.functional.test.ts

@@ -40,14 +40,14 @@ const pythonFilesDir = path.join(workspaceDir, 'pythonFiles', 'linting');
 const fileToLint = path.join(pythonFilesDir, 'file.py');
 
 const linterConfigDirs = new Map<LinterId, string>([
-    ['flake8', path.join(pythonFilesDir, 'flake8config')],
-    ['pycodestyle', path.join(pythonFilesDir, 'pycodestyleconfig')],
-    ['pydocstyle', path.join(pythonFilesDir, 'pydocstyleconfig27')],
-    ['pylint', path.join(pythonFilesDir, 'pylintconfig')]
+    [LinterId.Flake8, path.join(pythonFilesDir, 'flake8config')],
+    [LinterId.PyCodeStyle, path.join(pythonFilesDir, 'pycodestyleconfig')],
+    [LinterId.PyDocStyle, path.join(pythonFilesDir, 'pydocstyleconfig27')],
+    [LinterId.PyLint, path.join(pythonFilesDir, 'pylintconfig')]
 ]);
 const linterConfigRCFiles = new Map<LinterId, string>([
-    ['pylint', '.pylintrc'],
-    ['pydocstyle', '.pydocstyle']
+    [LinterId.PyLint, '.pylintrc'],
+    [LinterId.PyDocStyle, '.pydocstyle']
 ]);
 
 const pylintMessagesToBeReturned: ILintMessage[] = [

src/test/linters/linter.availability.unit.test.ts

@@ -26,7 +26,7 @@ import {
 import { Common, Linters } from '../../client/common/utils/localize';
 import { AvailableLinterActivator } from '../../client/linters/linterAvailability';
 import { LinterInfo } from '../../client/linters/linterInfo';
-import { IAvailableLinterActivator, ILinterInfo } from '../../client/linters/types';
+import { IAvailableLinterActivator, ILinterInfo, LinterId } from '../../client/linters/types';
 
 // tslint:disable:max-func-body-length no-any
 suite('Linter Availability Provider tests', () => {
@@ -257,7 +257,7 @@ suite('Linter Availability Provider tests', () => {
                 this.testIsEnabled = enabled;
                 return Promise.resolve();
             }
-        })(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']);
+        })(Product.pylint, LinterId.PyLint, configServiceMock.object, ['.pylintrc', 'pylintrc']);
 
         const notificationPromptEnabled = TypeMoq.Mock.ofType<IPersistentState<boolean>>();
         factoryMock
@@ -856,6 +856,6 @@ function getDependenciesForAvailabilityTests(): [
         TypeMoq.Mock.ofType<IWorkspaceService>(),
         TypeMoq.Mock.ofType<IConfigurationService>(),
         TypeMoq.Mock.ofType<IPersistentStateFactory>(),
-        new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc'])
+        new LinterInfo(Product.pylint, LinterId.PyLint, configServiceMock.object, ['.pylintrc', 'pylintrc'])
     ];
 }

src/test/linters/mypy.unit.test.ts

@@ -8,7 +8,7 @@
 import { expect } from 'chai';
 import { parseLine } from '../../client/linters/baseLinter';
 import { REGEX } from '../../client/linters/mypy';
-import { ILintMessage } from '../../client/linters/types';
+import { ILintMessage, LinterId } from '../../client/linters/types';
 
 // This following is a real-world example. See gh=2380.
 // tslint:disable-next-line:no-multiline-string
@@ -57,7 +57,7 @@ suite('Linting - MyPy', () => {
             ]
         ];
         for (const [line, expected] of tests) {
-            const msg = parseLine(line, REGEX, 'mypy');
+            const msg = parseLine(line, REGEX, LinterId.MyPy);
 
             expect(msg).to.deep.equal(expected);
         }