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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, looks simpler now, just a few more nits.



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:7
+interface TokenColorRule {
+  scope: string, textColor: string,
+}
----------------
nit: we need documentation for the fields.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:11
+// Gets a TextMate theme and all its included themes by its name.
+async function loadTheme(themeName: string): Promise<TokenColorRule[]> {
+  const extension =
----------------
do we need `async` here? since we don't use any `await` in the function body.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:25
+
+  const extensionInfo = extension.packageJSON.contributes.themes.find(
+      (theme: any) => theme.id === themeName || theme.label === themeName);
----------------
nit: name it `themeData` or `themeInfo`?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:31
+/**
+ * Recursively parse the TM grammar at fullPath. If there are multiple TM scopes
+ * of the same name in the include chain only the earliest entry of the scope is
----------------
nit: textmate grammar is another different thing, the file is the textmate **theme**.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:35
+ * @param fullPath The absolute path to the theme.
+ * @param scopeSet A set containing the name of the scopes that have already
+ *     been set.
----------------
nit: maybe `SeenScopes`?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:42
+    scopeSet = new Set();
+  // FIXME: Add support for themes written as .thTheme.
+  if (path.extname(fullPath) === '.tmTheme')
----------------
nit: th => tm


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:45
+    return [];
+  // If there is an error opening a file, the TM files that were correctly found
+  // and parsed further up the chain should be returned. Otherwise there will be
----------------
I think it would be more suitable to move this comment in the `catch` block below, we don't throw this error.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:94
+      }
+      resolve(data);
+    });
----------------
nit: return resolve(data).


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc:3
+    // Some comment
+    "include": "secondIncludedTheme.jsonc",
+    "tokenColors": [
----------------
nit: any reason use `jsonc` not `json`?



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc:2
+{
+    "include": "tmThemeInclude.tmTheme",
+    "tokenColors": [
----------------
since we don't support `tmTheme` now, I'd prefer just dropping it from the test.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc:3
+    // Some comment
+    "include": "firstIncludedTheme.jsonc",
+    "name": "TestTheme",
----------------
I think using one-level test is sufficient, how about the tests below?

- simpleTheme.json (a simple non-include theme)
- includeTheme.json (one-level include them)


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:1
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';
----------------
this comment doesn't seem to provide much information, I'd remove it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738