-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add bandit to supported linters #2775
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
Conversation
|
The order of the linters in various data structures was kind of random. Should I alphabetize or standardize this? |
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.
Over all a great PR, you seem to have done everything and beyond.
My one suggestion is to remove the ability to configure the mapping for HIGH, MEDIUM and LOW, for now lets just assume they map to Error, Warning and Information, respectively.
src/client/common/configSettings.ts
Outdated
| banditCategorySeverity: { | ||
| LOW: DiagnosticSeverity.Error, | ||
| MEDIUM: DiagnosticSeverity.Error, | ||
| HIGH: DiagnosticSeverity.Error |
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.
Seems to be wrong, all map to Error.
Do we need this mapping?
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.
whoops, fixed
package.json
Outdated
| ], | ||
| "scope": "resource" | ||
| }, | ||
| "python.linting.banditCategorySeverity.HIGH": { |
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.
Do we need these three severities?
Can't we hardcode for now, this way if users request changes we can add it.
I feel that's better than adding configuration settings that might not ever get used.
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.
done
src/client/linters/bandit.ts
Outdated
| protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise<ILintMessage[]> { | ||
| const messages = await this.run(['-f', 'custom', '--msg-template', '{line},0,{severity},{test_id}:{msg}', document.uri.fsPath], document, cancellation); | ||
| messages.forEach(msg => { | ||
| msg.severity = this.parseMessagesSeverity(msg.type, this.pythonSettings.linting.banditCategorySeverity); |
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.
I think its best to hard code the severity mapping for now, and introduce additional settings later if & when users request for the ability to change the mapping (meaning of the errors).
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.
done
src/client/common/types.ts
Outdated
| LOW: DiagnosticSeverity.Error; | ||
| MEDIUM: DiagnosticSeverity.Error; | ||
| HIGH: DiagnosticSeverity.Error; | ||
| } |
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.
this needs to be:
export interface IBanditCategorySeverity {
LOW: DiagnosticSeverity;
MEDIUM: DiagnosticSeverity;
HIGH: DiagnosticSeverity;
}
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.
fixed
|
@brettcannon /cc |
|
@DonJayamanne, thanks for the super fast review and feedback! will make those changes. |
|
@DonJayamanne Made all the changes. Added a fix as well for a bug in Bandit's message templating, documented here: PyCQA/bandit#371 and fixed on Bandit master will fix tests tomorrow |
src/test/.vscode/settings.json
Outdated
| "python.formatting.provider": "yapf", | ||
| "python.linting.pylintUseMinimalCheckers": false | ||
| "python.linting.pylintUseMinimalCheckers": false, | ||
| "python.pythonPath": "python" |
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.
You might want to remove python.pythonPath from the settings.
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.
done.
|
@demus |
|
@DonJayamanne should be all set |
For #2726
Any new/changed dependencies inpackage.jsonare pinned (e.g."1.2.3", not"^1.2.3"for the specified version)package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)