Skip to content

Conversation

@demus
Copy link

@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
Copy link

msftclas commented Oct 3, 2018

CLA assistant check
All CLA requirements met.

@demus
Copy link
Author

demus commented Oct 3, 2018

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

Copy link

@DonJayamanne DonJayamanne left a 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.

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

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?

Copy link
Author

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": {

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

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;
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@DonJayamanne
Copy link

@brettcannon /cc

@demus
Copy link
Author

demus commented Oct 3, 2018

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

@demus
Copy link
Author

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

"python.formatting.provider": "yapf",
"python.linting.pylintUseMinimalCheckers": false
"python.linting.pylintUseMinimalCheckers": false,
"python.pythonPath": "python"

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@DonJayamanne
Copy link

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

@demus
Copy link
Author

demus commented Oct 9, 2018

@DonJayamanne should be all set

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants