Skip to content

MRVA: Generate markdown summary for query with raw results#1318

Merged
shati-patel merged 4 commits intomainfrom
shati-patel/md-raw-result
Apr 27, 2022
Merged

MRVA: Generate markdown summary for query with raw results#1318
shati-patel merged 4 commits intomainfrom
shati-patel/md-raw-result

Conversation

@shati-patel
Copy link
Copy Markdown
Contributor

First step towards rendering markdown for non-interpreted results (see internal issue).

This PR adds new test data and creates a summary file (example here) 🖼️
I haven't yet added support for the rendering the actual results table (I'll do that in a follow-up PR 🔜 )

Checklist

N/A - internal only 🔐

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@shati-patel shati-patel force-pushed the shati-patel/md-raw-result branch from 630d962 to 2201572 Compare April 26, 2022 14:56
@shati-patel shati-patel marked this pull request as ready for review April 26, 2022 15:03
@shati-patel shati-patel requested review from a team as code owners April 26, 2022 15:03
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Sorry, ran out of time, will look at this more tomorrow!

/**
* Returns the number of (raw + interpreted) results for an analysis.
*/
export const getAnalysisResultCount = (analysisResults: AnalysisResults): number => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm this function doesn't really have much to do with SARIF so it feels like it doesn't belong to sarif.utils.ts. Should we move it to analysis-result.ts? (Or anywhere else you think is appropriate)

for (const analysisResult of analysesResults) {
if (analysisResult.interpretedResults.length === 0) {
// TODO: We'll add support for non-interpreted results later.
if (analysisResult.interpretedResults.length === 0 && !analysisResult.rawResults) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this really a check for 0 results? If so, we can move L23 to L17 and then do

if (resultsCount === 0) {
  continue;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you, good point! 🙈

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

🚢

@shati-patel shati-patel merged commit e12bf63 into main Apr 27, 2022
@shati-patel shati-patel deleted the shati-patel/md-raw-result branch April 27, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants