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