-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improves linting behavior per #722 #792
Conversation
Don't we need any tests for this new functionality? |
Done |
* @readonly | ||
*/ | ||
// tslint:disable-next-line:member-ordering | ||
readonly textDocuments: TextDocument[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this into the IDocumentMananager interface.
https://github.com/MikhailArkhipov/vscode-python/blob/ec4733253fbee51721cc1924a764c4a790171f86/src/client/common/application/types.ts#L314
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
this.interpreterService = serviceContainer.get<IInterpreterService>(IInterpreterService); | ||
|
||
this.disposables.push(this.interpreterService.onDidChangeInterpreter(() => this.engine.lintOpenPythonFiles())); | ||
this.disposables.push(vscode.workspace.onDidSaveTextDocument((e) => this.onDocumentSaved(e))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use IWorkspaceService instead of vscode.workspace
const fileName = path.basename(document.uri.fsPath).toLowerCase(); | ||
const watchers = linters.filter((info) => info.configFileNames.indexOf(fileName) >= 0); | ||
if (watchers.length > 0) { | ||
setTimeout(() => this.engine.lintOpenPythonFiles(), 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to lint just this one file, not all open files.
Else, if user has 10 python files opened, we'd be linting all 10 files everytime.
It gets worse with the auto-save feature in VS Code, as you start typing the document gets saved, i.e. linting all files again just while the user is typting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just call this.engine.lintDocument(document, 'auto')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is config change so more than a single file is affected.
Oops, forgot to hit the |
Codecov Report
@@ Coverage Diff @@
## master #792 +/- ##
==========================================
+ Coverage 63.26% 63.45% +0.18%
==========================================
Files 257 258 +1
Lines 11720 11764 +44
Branches 2081 2081
==========================================
+ Hits 7415 7465 +50
+ Misses 4297 4291 -6
Partials 8 8
Continue to review full report at Codecov.
|
/** | ||
* An event that is emitted when a [text document](#TextDocument) is opened. | ||
*/ | ||
readonly onDidOpenTextDocument: Event<TextDocument>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move these into the IDocumentManager interface (https://github.com/MikhailArkhipov/vscode-python/blob/c46856b7b5d10497f80b2daf35e3795372da0c29/src/client/common/application/types.ts#L314)
} | ||
|
||
// tslint:disable-next-line:no-any | ||
public async linkJupiterExtension(jupiter: vscode.Extension<any> | undefined): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, misspelled "Jupyter".
#722
Run Linter
command to force linting when, for example, user changed.pylintrc
elsewhere or user external editorMoved linting code from linter provider into new
linter engine