Skip to content

MRVA: Generate markdown for raw result tables#1323

Merged
shati-patel merged 4 commits intomainfrom
shati-patel/md-raw-results
Apr 29, 2022
Merged

MRVA: Generate markdown for raw result tables#1323
shati-patel merged 4 commits intomainfrom
shati-patel/md-raw-results

Conversation

@shati-patel
Copy link
Contributor

@shati-patel shati-patel commented Apr 28, 2022

Follow-up to #1318 for rendering raw results! This PR adds the actual table with results (see internal linked issue).

Some example rendered results here and here 🖼️ 🎨

(It looks kinda ugly, but maybe that's mainly because these example results and column names are fairly meaningless 😂)

Checklist

N/A - still 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 requested review from a team as code owners April 28, 2022 16:22
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

break;
}
// `|` characters break the table, so we need to escape them
return cellValue.replace('|', '\\|');

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding

This replaces only the first occurrence of '|'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah...thanks code scanning! I think you want to use replaceAll here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a line to one of the tests where there are multiple | in the same table row?

Copy link
Contributor Author

@shati-patel shati-patel Apr 28, 2022

Choose a reason for hiding this comment

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

Ah...thanks code scanning! I think you want to use replaceAll here.

I tried that, but then I got this mysterious error locally 🤨
image

Good idea about updating the tests! Will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I broke things more 😅 Will come back to this tomorrow!

Copy link
Contributor

Choose a reason for hiding this comment

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

replaceAll was introduced in node 15. And the package.json requires node >=14.17.1. So, I'm guessing that your local node version is too old.

Either:

  1. We change the requirement to node 15 or greater (double check exactly which version the function was added)
  2. Do multi-replacement in a different way. It should work if you call replace with a regex and use a global tag. eg- /\\|/g (this regex may not be right...., but it's something like that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right about needing to upgrade! (package.json updated in #1326)

I'm still slightly confused why replaceAll worked fine in other places like here: https://github.com/github/vscode-codeql/blob/main/extensions/ql-vscode/src/remote-queries/view/FileCodeSnippet.tsx#L24 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

At runtime, vscode is currently using node v16.13.0, so we can use it in the web view and in production. It's just that we could't use replaceAll in tests since our test version of node was too old. And we are not running any tests on our web view. 😢

@charisk
Copy link
Contributor

charisk commented Apr 29, 2022

Give me a shout if you need any help getting things green!

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

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

LGTM!

@shati-patel shati-patel merged commit ed84825 into main Apr 29, 2022
@shati-patel shati-patel deleted the shati-patel/md-raw-results branch April 29, 2022 13:58
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.

3 participants