-
Notifications
You must be signed in to change notification settings - Fork 573
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: sarif descirption change #1428
Conversation
87feb1c
to
13b5b6f
Compare
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.
looks good to me.
@@ -1,4 +1,5 @@ | |||
import * as sarif from 'sarif'; | |||
import upperFirst = require('lodash/upperFirst'); |
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 would like to recommend import { upperFirst } from 'lodash';
instead of import upperFirst = require('lodash/upperFirst');
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.
Would be nice also to fix the commit message to something that is more related to what you have added into the description of your pr:
small change to SARIF output description, severity to look the same in IaC and containers.
To give more context to our users of what is happening
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 was actually a change requested on the PR that introduced this functionality, I originally used your suggestion :) Don't mind changing if needed
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 are reducing the usage of require
giving the preference to import
@@ -1,4 +1,5 @@ | |||
import * as sarif from 'sarif'; | |||
import upperFirst = require('lodash/upperFirst'); |
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 can't use import?
import upperFirst = require('lodash/upperFirst'); | |
import upperFirst from 'lodash/upperFirst'; |
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 solution would not help because upperFirst
was exported as export = upperFirst;
and in order to allow it we need to change tsconfig.json
compilerOptions and add a new property
"esModuleInterop": true,
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.
Was a request in the original PR. Considering the two comments on this now, changed 👍
101e8bd
to
9702a0e
Compare
9702a0e
to
4abfb97
Compare
🎉 This PR is included in version 1.405.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What does this PR do?
Small change to SARIF output description, severity to look the same in IaC and containers.