-
Notifications
You must be signed in to change notification settings - Fork 329
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 baseline metrics for lines of code #459
Conversation
c5d6cae
to
d2b4652
Compare
5c4018b
to
d6f3eb6
Compare
d2b4652
to
c4a84a9
Compare
d6f3eb6
to
76702a2
Compare
@@ -145,6 +146,15 @@ export async function runQueries( | |||
): Promise<QueriesStatusReport> { | |||
const statusReport: QueriesStatusReport = {}; | |||
|
|||
// count the number of lines in the background | |||
const locPromise = countLoc( |
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 is clever, so the promise it's only resolved when it's used but is used in potentially multiple places. Worth noting that in practice there will always be some queries to evaluate so we'll always end up using this promise. It's an error for there not to be any queries to analyse and it would have error-ed back in the init step. Up to you if you therefore want to leave this as it is or potentially simplify it.
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.
The main reason why I'm doing this is so that the line counting can happen in "parallel". On large projects, counting can take 10-20s and there's lots of disk IO, so it's nice to be able to run this while other things are happening.
74c1aef
to
5201fb4
Compare
5201fb4
to
a1e16be
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.
Nice and clear. Couple of recommendations based on CodeQL conventions for labelling languages.
0ce85b6
to
674720b
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.
LGTM from my point of view. Probably best let @adityasharad also review the recent changes.
674720b
to
28900de
Compare
This commit uses a third party library to estimate the lines of code in a database that is to be analyzed by codeql. The estimate uses the same includes and excludes globs for determining which files should be counted. The lines of code count is returned by language and injected into the SARIF as `baseline` property in the `${language}/summary/lines-of-code` metric.
28900de
to
998f472
Compare
This commit uses a third party library to estimate the lines of code in
a database that is to be analyzed by codeql.
The estimate uses the same includes and excludes globs for determining
which files should be counted.
The lines of code count is returned by language and injected into the
SARIF as appropriate.
Currently, this PR adds the LoC data in themetricResults
property of the sarif in a blob like this:We haven't agreed on what this will look like, so injecting the metric may change.We've decided that the lines of code will be injected into metrics with id like this:
${language}/summary/lines-of-code
and a newbaseline
property is added to the metric. Languages that we have a count for, but no metric, will be ignored.Merge / deployment checklist