hokein added inline comments.
================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:6 + +export namespace SemanticHighlighting { +interface ScopeColorRule { ---------------- I think we should not use the namespace in typescript. The `namespace` in TS refers to [“Internal modules”](https://www.typescriptlang.org/docs/handbook/namespaces.html). Each TS file is a separate module, so semantically namespace your code, use separate files. For semantic highlighting feature, I think we put all into one file `semantic-highlighting.ts`. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:7 +export namespace SemanticHighlighting { +interface ScopeColorRule { + scope: string, textColor: string, ---------------- TokenColorRule seems a bit clearer to me. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:24 + */ +export function themesToScopeColors(themeContents: any[]): ScopeColorRule[] { + const ruleMap: Map<string, string> = new Map(); ---------------- we are introducing 2~3 helper functions here, I think we could simplify it like below. ``` function loadTheme(themeName) : Promise<ScopeColorRule[]> { // iterate all extensions and find the file path of the theme // call parseThemeFile(). } function parseThemeFile(themeFilePath) : Promise<ScopeColorRule[]> { // read the file and parse the file content (recusively) } ``` And for the test, we could save a small theme file in the test directory, and pass the file path to the `parseThemeFile` function. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:75 + const contents = await readFileText(fullPath); + // FIXME: Add support for themes written as .thTheme. + const parsed = jsonc.parse(contents); ---------------- nit: we should filter the `.tmTheme` out in the code (just checking the extension of the fullpath). ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:93 +function readFileText(path: string): Promise<string> { + return new Promise((res, rej) => { + fs.readFile(path, 'utf8', (err, data) => { ---------------- nit: use the full name for the parameters, `resolve`, `reject`. ================ Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:96 + if (err) { + rej(err); + } ---------------- if we have err, we will run both `rej` and `res`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65738/new/ https://reviews.llvm.org/D65738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits