-
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
Fix linters to make use of the new python code execution framework #360
Fix linters to make use of the new python code execution framework #360
Conversation
Archive of 0.7.0
* 'master' of https://github.com/Microsoft/vscode-python: Fixes #56 list all environments (#219) Fixes #57 Disable activation on debugging (#220) Fixes #26 Do not run linters when linters are disabled (#222)
* upstream/master: Fix typo in README.md (#252) Disable linter without workspaces (#241)
* upstream/master: Document contribution to the code along with coding standards (#321)
* upstream/master: Add Simplified Chinese translation of commands (#240)
* upstream/master: Fix package.json (#347)
* upstream/master: #34, #110 - suppress Intellisense in strings and comments (#339) Re-factor code python execution framework (#345)
package.json
Outdated
@@ -1586,5 +1568,10 @@ | |||
"typescript": "^2.6.2", | |||
"typescript-formatter": "^6.0.0", | |||
"vscode": "^1.1.5" | |||
}, | |||
"__metadata": { |
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.
What's this for?
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.
Damn thing keeps getting added. No idea. Tried to find this, gave up. Added by vscode.
src/client/linters/baseLinter.ts
Outdated
} | ||
} | ||
protected async parseMessages(output: string, document: TextDocument, token: CancellationToken, regEx: string) { | ||
const outputLines = output.split(/\r?\n/g); |
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 really need to stick this as a utility function somewhere since we use it in so many places and it's easy enough to screw up that regex by leaving something out.
public handleError(expectedFileName: string, fileName: string, error: Error, resource: Uri) { | ||
this._errorHandlers.some(handler => handler.handleError(expectedFileName, fileName, error, resource)); | ||
public handleError(error: Error, resource: Uri, execInfo: ExecutionInfo): Promise<boolean> { | ||
return this.handlder.handleError(error, resource, execInfo); |
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.
handlder
?
And if that is an error, why isn't TypeScript complaining about the attempt to use a non-existent attribute?
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.
It is an existing attribute, just a typo. Fixed typo
@@ -34,12 +33,17 @@ export abstract class BaseTestManager implements ITestManager { | |||
private _status: TestStatus = TestStatus.Unknown; | |||
private testDiscoveryCancellationTokenSource?: vscode.CancellationTokenSource; | |||
private testRunnerCancellationTokenSource?: vscode.CancellationTokenSource; | |||
private installer: Installer; | |||
private _installer: IInstaller; |
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.
Why the leading underscore if it's private? I notice there are some private attributes with one, others that don't.
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.
Have a property named installer
(getter property using the _installer
as a backing property.
Nothing to do here.
@brettcannon fixed. Will start replacing .split(\r\nr...) as i go along. |
Fixes #351