Refactor language parsing #144
Conversation
robertbrignull
force-pushed
the
language_parsing
branch
from
65bd4d6 to
591359c
Compare
src/languages.ts
Outdated
| @@ -0,0 +1,39 @@ | |||
| // All the languages supported by CodeQL | |||
| export const ALL_LANGUAGES = ['csharp', 'cpp', 'go', 'java', 'javascript', 'python'] as const; | |||
There was a problem hiding this comment.
Would it make more sense to use an enum here? eg-
enum Language {
csharp = 'csharp',
cpp = 'cpp',
...
}
This would likely simplify the code somewhat and be more idiomatic typescript.
There was a problem hiding this comment.
The reason I did it this way was so you can check if something is part of the list at compiletime and runtime. According to https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-4.html#string-enums this isn't for string-valued enums. So I shouldn't be able to do something like if (language in Language)?
However, I tried it and added some tests (which I should have added from the start) and it works. So as far as I can tell it's all good. Do you agree?
mveytsman
left a comment
mveytsman
left a comment
There was a problem hiding this comment.
I love this! Great refactor and use of enums!
Depends on #140 so only the last commit is new to this PR.This PR attempts to make language parsing more localised instead of being spred out over multiple places in multiple files. This should hopefully be a benefit if we ever add another offical language, or if codeql moves to fully 3rd-party extractors and we need to rethink languages entirely.
Also uses the
Languagetype in more places, but thankfully all the call sites were already passing that type so it's all good.Also starts using the
import { foo } from './bar';import syntax more. So far in this repository we've been exclusively doingimport * as bar from './bar';but I don't think there was any big reason for that.Merge / deployment checklist