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

Add bandit to supported linters #2775

Merged
merged 16 commits into from Oct 9, 2018

Conversation

Projects
None yet
3 participants
@demus

demus commented Oct 3, 2018

For #2726

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • [x ] Has a news entry file (remember to thank yourself!)
  • Unit tests & system/integration tests are added/updated
  • Any new/changed dependencies in package.json are pinned (e.g. "1.2.3", not "^1.2.3" for the specified version)
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
@msftclas

This comment has been minimized.

msftclas commented Oct 3, 2018

CLA assistant check
All CLA requirements met.

@demus

This comment has been minimized.

demus commented Oct 3, 2018

The order of the linters in various data structures was kind of random. Should I alphabetize or standardize this?

@DonJayamanne

DonJayamanne requested changes Oct 3, 2018 edited

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.

banditCategorySeverity: {
LOW: DiagnosticSeverity.Error,
MEDIUM: DiagnosticSeverity.Error,
HIGH: DiagnosticSeverity.Error

This comment has been minimized.

@DonJayamanne

DonJayamanne Oct 3, 2018

Seems to be wrong, all map to Error.
Do we need this mapping?

This comment has been minimized.

@demus

demus Oct 3, 2018

whoops, fixed

],
"scope": "resource"
},
"python.linting.banditCategorySeverity.HIGH": {

This comment has been minimized.

@DonJayamanne

DonJayamanne Oct 3, 2018

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.

This comment has been minimized.

@demus
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);

This comment has been minimized.

@DonJayamanne

DonJayamanne Oct 3, 2018

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).

This comment has been minimized.

@demus
LOW: DiagnosticSeverity.Error;
MEDIUM: DiagnosticSeverity.Error;
HIGH: DiagnosticSeverity.Error;
}

This comment has been minimized.

@DonJayamanne

DonJayamanne Oct 3, 2018

this needs to be:

export interface IBanditCategorySeverity {
    LOW: DiagnosticSeverity;
    MEDIUM: DiagnosticSeverity;
    HIGH: DiagnosticSeverity;
}

This comment has been minimized.

@demus
@DonJayamanne

This comment has been minimized.

DonJayamanne commented Oct 3, 2018

@demus

This comment has been minimized.

demus commented Oct 3, 2018

@DonJayamanne, thanks for the super fast review and feedback! will make those changes.

@demus

This comment has been minimized.

demus commented Oct 3, 2018

@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

demus added some commits Oct 3, 2018

@@ -18,5 +18,6 @@
"python.linting.mypyEnabled": false,
"python.linting.banditEnabled": false,
"python.formatting.provider": "yapf",
"python.linting.pylintUseMinimalCheckers": false
"python.linting.pylintUseMinimalCheckers": false,
"python.pythonPath": "python"

This comment has been minimized.

@DonJayamanne

DonJayamanne Oct 9, 2018

You might want to remove python.pythonPath from the settings.

This comment has been minimized.

@demus
@DonJayamanne

This comment has been minimized.

DonJayamanne commented Oct 9, 2018

@demus
Please could you resolve the code merge issue.
I'd like to merge this in today.

@demus

This comment has been minimized.

demus commented Oct 9, 2018

@DonJayamanne should be all set

@DonJayamanne DonJayamanne merged commit 94a246b into Microsoft:master Oct 9, 2018

1 of 3 checks passed

VSCode-Python-CI in progress
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment