[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

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