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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
