Skip to content

Queries panel: display the query language in the UI#2472

Merged
shati-patel merged 5 commits intomainfrom
shati-patel/display-query-language
Jun 5, 2023
Merged

Queries panel: display the query language in the UI#2472
shati-patel merged 5 commits intomainfrom
shati-patel/display-query-language

Conversation

@shati-patel
Copy link
Copy Markdown
Contributor

Updates the QueryTreeViewItem, so that it can display the language of a CodeQL query (and display it in the UI). This involved updating the shared FileTreeNode class to accept a generic data property.

This will hook in to #2471 (which gets the actual query language). For now, I've hard-coded a placeholder string:

Queries panel showing a language label next to each query

Checklist

N/A—internal only 🌍 See internal linked issue for more details! 🤫

  • 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 marked this pull request as ready for review June 1, 2023 16:56
@shati-patel shati-patel requested a review from a team as a code owner June 1, 2023 16:56
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

I have one tiny suggestion which you are free to take or leave. But this PR and approach looks good to me. Admittedly I helped with developing it.

From my point of view you're welcome to get another review, or go ahead with this if you like. I don't think any of the changes here are hard to review, so there's not really any risk.

constructor(_path: string, _name: string) {
super(_path, _name);
export class FileTreeLeaf<T = undefined> extends FileTreeNode<T> {
constructor(_path: string, _name: string, _data: T) {
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.

Suggested change
constructor(_path: string, _name: string, _data: T) {
constructor(_path: string, _name: string, _data?: T) {

If we make the _data arg optional here then we can revert the change to qltest-discovery.ts and avoid changing that file at all in this PR, but not change behaviour.

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.

Thanks, made that change! Should we also make data optional in the FileTreeNode itself?

@shati-patel
Copy link
Copy Markdown
Contributor Author

From my point of view you're welcome to get another review, or go ahead with this if you like. I don't think any of the changes here are hard to review, so there's not really any risk.

One more thing I was wondering: is there a useful way to test this change? There are quite a few optional/undefined params, so perhaps it would be helpful to test some different combinations of setting them 🤔

@shati-patel
Copy link
Copy Markdown
Contributor Author

From my point of view you're welcome to get another review, or go ahead with this if you like. I don't think any of the changes here are hard to review, so there's not really any risk.

One more thing I was wondering: is there a useful way to test this change? There are quite a few optional/undefined params, so perhaps it would be helpful to test some different combinations of setting them 🤔

Update (as discussed offline): there doesn't seem to be much useful to test here, so we'll wait until we've also hooked up the "backend" in #2471 and then test things as a whole.

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

LGTM and happy to merge this part of the work.

As noted on the other PR, determining the language is current a bit slow so we might need to look into fixing that or different ways of doing that, but this PR for showing the language in the UI will still be needed. So there's no harm in merging it now. It's all feature flagged right now.

In terms of testing, I think manual testing will have to do for now. Once we've got the CLI side hooked up we could add a CLI integration test of the full flow.

@shati-patel shati-patel merged commit af9af99 into main Jun 5, 2023
@shati-patel shati-patel deleted the shati-patel/display-query-language branch June 5, 2023 10:08
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