Skip to content

Handle codeql-pack.yml files everywhere#2054

Merged
koesie10 merged 3 commits intomainfrom
koesie10/codeql-pack-yml
Feb 9, 2023
Merged

Handle codeql-pack.yml files everywhere#2054
koesie10 merged 3 commits intomainfrom
koesie10/codeql-pack-yml

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Feb 7, 2023

This will add support for the codeql-pack.yml filename in all places where we currently support qlpack.yml. It centralizes the list of the supported filenames in a single place and a method that can figure out the correct filename to use.

Checklist

  • 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.

This will add support for the `codeql-pack.yml` filename in all places
where we currently support `qlpack.yml`. It centralizes the list of the
supported filenames in a single place and a method that can figure out
the correct filename to use.
@koesie10 koesie10 added the secexp label Feb 7, 2023
@koesie10 koesie10 marked this pull request as ready for review February 8, 2023 09:08
@koesie10 koesie10 requested review from a team as code owners February 8, 2023 09:08
Copy link
Copy Markdown
Contributor

@elenatanasoiu elenatanasoiu left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for doing this. I left some small suggestions but they're not blocking.

Comment thread extensions/ql-vscode/src/helpers.ts Outdated
}
}

return undefined;
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.

Could we have a test for this new function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍 , I've added a test for this function. I've also moved the function to the pure directory because this doesn't actually need VSCode and that makes it clearer that this can be a unit test.

) {
if (!qlPackFile) {
return true;
}
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.

I see we don't have a test file for quick-query functions so this is a stretch. If the setup isn't terribly complicated, do you think it's worth adding one to test what this is doing?

I do see we have a queries.test.ts file which I guess covers some of the quick-query functionality.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I could add a test for this function, but I don't really know what input values it would actually receive. I'm not quite sure when the function should be returning true or false or what it's purpose is, so I'm hesitant to add tests based purely on its implementation, rather than on its intended behaviour.

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.

Quick query is a feature where the IDE generates a bare-bones qlpack in a temporary directory that is tailored to the current language. It is meant to be semi-ephemeral.

When you call the quick query command, the extension checks if the query query pack exists already and if it is for the correct language of the current database.

Comment thread extensions/ql-vscode/test/unit-tests/pure/ql.test.ts
@koesie10 koesie10 merged commit 2122737 into main Feb 9, 2023
@koesie10 koesie10 deleted the koesie10/codeql-pack-yml branch February 9, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants