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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

mostly good, please update the patch description.



================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:6
+
+interface TokenColorRule {
+  // Scope is the TextMate scope for this rule.
----------------
the interface also needs a documentation.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:7
+interface TokenColorRule {
+  // Scope is the TextMate scope for this rule.
+  scope: string;
----------------
maybe: `// A TextMate scope that specifies the context of the token, e.g. "entity.name.function.cpp"`





================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:9
+  scope: string;
+  // textColor is the color tokens of this scope should have.
+  textColor: string;
----------------
I know the current name was my suggestion, but rethinking the name here, I think it'd be better to align with the name used by vscode (to avoid confusion), just use `foreground`.


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:13
+
+// Gets a TextMate theme and all its included themes by its name.
+function loadTheme(themeName: string): Promise<TokenColorRule[]> {
----------------
just `// Get all token color rules provided by the theme`


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:34
+/**
+ * Recursively parse the TM theme 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: I think there is no need to explicitly mention the `Recursively`.

we are using `TextMate` and `TM` to refer the same thing in the source file, could we use a consistent way (just use `TextMate`)?


================
Comment at: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc:3
+    // Some comment
+    "include": "secondIncludedTheme.jsonc",
+    "tokenColors": [
----------------
jvikstrom wrote:
> hokein wrote:
> > nit: any reason use `jsonc` not `json`?
> > 
> By default vscode will bind .json files to normal json files which don't allow comments. So if you'd try to run the tests without having set .json to bind to json with comments than it will be a "compile error" because of vscode not allowing comments in .json.
>  
thanks for the explanations, fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65738