- Notifications
You must be signed in to change notification settings - Fork355
Do not artificially restrict the set of supported languages#779
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This is generally a good idea, IMO, especially as we start building out more languages. |
| @@ -299,7 +299,7 @@ test("load non-empty input", async (t) => { | |||
| // And the config we expect it to parse to | |||
| const expectedConfig: configUtils.Config = { | |||
| languages: [Language.javascript], | |||
| languages: [KnownLanguage.javascript], | |||
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.
I think we should have a new unit test here for a language is not a known language, but it is valid since it was returned by a mocked call tocodeql resolve languages.
| @@ -1172,7 +1174,7 @@ function shouldCombinePacks(packsInput?: string): boolean { | |||
| function combinePacks(packs1: Packs, packs2: Packs): Packs { | |||
| const packs = {}; | |||
| for (const lang of Object.keys(packs1)) { | |||
| packs[lang] = packs1[lang].concat(packs2[lang] || []); | |||
| packs[lang] = packs1[lang]?.concat(packs2[lang] || []); | |||
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.
I think this is what we want to do in order to ensure we never setpacks[lang] to undefined.
| packs[lang]=packs1[lang]?.concat(packs2[lang]||[]); | |
| packs[lang]=(packs1[lang]||[]).concat(packs2[lang]||[]); |
| (process.env["CODEQL_EXTRACTOR_GO_BUILD_TRACING"] === "on" && | ||
| language ===Language.go) | ||
| language ===KnownLanguage.go) |
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.
So any nonKnownLanguage is assumed to be a scanned language? Is that correct?
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.
Yes. The default could go either way though.
For full flexibility, the action could take a parameter that signals the desired mode.
Will the extra extractors need to be packaged with the distribution, or are you expecting extractors to be resolvable from user supplied qlpacks? |
If you modify the codeql search path appropriately, you can use your own extractor in the action. We do not document how though. (Seehttps://github.com/github/codeql-ql/blob/main/.github/workflows/nightly-changes.yml#L82) |
walid97t left a comment
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.
go ahead
Description
The hardcoded set of supported languages in
languages.tsmakes it hard to experiment with new languages as custom branch with a modifiedlanguages.tsis required for every new language.This PR makes it easier to experiment with new languages by making the
codeql-actionindifferent to the language choice, except for someadditional support for known languages.Implementation
The old implementation throws an early error if the user-provided language is not in the hardcoded set. As an alternative, a
Languageis now just a lowercase string, and an early error is only produced if that language is not in the output ofcodeql resolve languages. The oldLanguageenum is now namedKnownLanguage, and is used in the places whereadditional support for a specific language is desired.This should be fully backwards compatible, as the implementation now should throw an early exception in fewer situations. This may be detrimental to how early and well users are informed of input mistakes. On the other hand, it is consistent with how the provided
codeqland environment behave.PMs may want to keep the tight coupling between the codeql-action and the GitHub-controlled CodeQL repositories, I do not.
Testing
I will use this branch for some internal experiments for a short while. Hopefully, the many changes wont result in too many conflicts when we are ready to merge.
Merge / deployment checklist