Add first zig scope facet - #3111
Conversation
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
AndreasArvidsson
left a comment
There was a problem hiding this comment.
I've gone through all your new tests and left comments. Please use typescript as a reference language to help you figure out the correct ranges. We do have a scope visualizer on the docs page to help you with this:
Per language:
https://www.cursorless.org/docs/user/languages/typescript/
Per scope:
https://www.cursorless.org/docs/contributing/scopes/argumentList/
The tests are also failing which you need have a look at.
There was a problem hiding this comment.
bbbis missing as a scope- The removal range of
aaais incorrect - The insertion delimiter is incorrect for a multiline argument
There was a problem hiding this comment.
Please use another language, eg javascript, as a reference:
https://github.com/cursorless-dev/cursorless/blob/d21522be750a55b70b50ac251300bfd6dd4baace/data/fixtures/scopes/javascript.core/argument/argument.actual.multiLine.scope
There was a problem hiding this comment.
bbbis missing as a scope- The removal range of
aaais incorrect - The insertion delimiter is incorrect
There was a problem hiding this comment.
Please include a second argument so we get a separate removal range in this scope test
| >-----< | ||
| 0| foo(); | ||
|
|
||
| [Insertion delimiter] = " " |
There was a problem hiding this comment.
The insertion delimiter should be an empty string
| >-------------< | ||
| 0| foo(aaa, bbb); | ||
|
|
||
| [Insertion delimiter] = " " |
There was a problem hiding this comment.
The insertion delimiter should be ", "
|
|
||
| [#3 Removal] = 0:14-0:23 | ||
| >---------< | ||
| 0| fn foo(aaa: u8, bbb: u8) void {} |
| @@ -0,0 +1,62 @@ | |||
| fn foo(aaa: u8, bbb: u8) void {} | |||
There was a problem hiding this comment.
Please remove the arguments for this tests. Now the file is unnecessarily complicated.
|
|
||
| [Removal] = 0:11-0:14 | ||
| >---< | ||
| 0| const foo: u8 = 0; |
There was a problem hiding this comment.
Removal range should be : u8
| @@ -0,0 +1,20 @@ | |||
| const foo = "bar"; | |||
There was a problem hiding this comment.
Assignment should be assigning a value to an already existing variable. Please rename this file to value.variable
|
|
||
| [Removal] = 0:11-0:17 | ||
| >------< | ||
| 0| const foo = "bar"; |
There was a problem hiding this comment.
Removal range should be = "bar"
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed6c426bb2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| foo( | ||
| aaa, | ||
| bbb | ||
| ); |
There was a problem hiding this comment.
Use a function declaration for formal list fixture
This fixture is named argumentList.formal.multiLine, but the sample code is a call expression (foo(...)) rather than a function declaration. The Zig query for formal argument lists only matches function_declaration (parameters), so this fixture won’t exercise the intended scope and can let regressions slip or produce misleading docs/tests for formal lists. Consider changing the snippet to a fn foo(... ) declaration so the fixture actually validates the formal list behavior.
Useful? React with 👍 / 👎.
| [#2 Content] = 0:12-0:14 | ||
| >--< | ||
| 0| fn foo(aaa: u8, bbb: u8) void {} |
There was a problem hiding this comment.
Don’t include parameter types in return-type fixture
The type.return fixture includes additional captures for the parameter types ([#2 Content] = 0:12-0:14, etc.). That makes the fixture assert that parameter types are part of the return-type scope, which contradicts the facet’s intent and will either fail tests or mask a broken return-type query by validating the wrong targets. The fixture should only contain the return type range.
Useful? React with 👍 / 👎.
|
Sorry I haven't gotten to this yet — life threw a few curveballs at me at once. I really appreciate the thorough reviews and hopefully I'll be able to get back to this and wrap it up soon 🙏 |
|
No stress. Let's revisit whenever you have the time. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c3428d978
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (call_expression | ||
| function: (_) @functionCallee | ||
| (arguments | ||
| (_) @argumentOrParameter |
There was a problem hiding this comment.
Configure delimiters on the captured actual arguments
The first call_expression match captures each actual argument without assigning an insertion delimiter, so it wins over the later delimiter-aware match; the committed single-line fixture confirms the resulting delimiter is " " rather than ", ". Consequently, inserting or bringing an argument beside aaa in foo(aaa, bbb) produces invalid Zig such as foo(aaa newArg, bbb), and multiline calls have the same problem. Capture actual arguments only through the delimiter-configured pattern (using the arguments node as its multiline condition).
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
| @@ -0,0 +1,10 @@ | |||
| foo(aaa, bbb); | |||
There was a problem hiding this comment.
Put the Zig fixtures in the discovered scope-test directory
All newly added Zig fixtures are under data/fixtures/scopes, but the scope-test discovery code reads only resources/fixtures/scopes; a repository-wide inspection found no discovery path for this new data directory. As a result, pnpm test silently runs none of these 21 fixtures, so query regressions—including the incorrect delimiter currently recorded by these files—are not checked.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
|
|
||
| const { supported, notApplicable, unsupported } = ScopeSupportFacetLevel; | ||
|
|
||
| export const zigScopeSupport: LanguageScopeSupportFacetMap = { |
There was a problem hiding this comment.
Register the Zig support table with its consumers
This exports a Zig support table but never imports it into languageScopeSupport, and LanguageId/languageReferences also omit zig. Therefore the scope-test recorder cannot offer Zig, the missing-scope documentation excludes it, and moving these fixtures into the discovered directory would make scope coverage fail at getLanguageId("zig"); add Zig to both registries when introducing this table.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
| @@ -0,0 +1,5 @@ | |||
| import { Language } from "./components/Language"; | |||
There was a problem hiding this comment.
Place the Zig language page in the documentation package
This page is added beneath a new packages/cursorless-org-docs tree, while the built documentation and every existing language page are under packages/app-web-docs; a repository-wide search also finds no components/Language module at this import location. The documentation build therefore never includes this Zig page, so it needs to be generated in the actual docs package using that package's existing language-page structure.
AGENTS.md reference: AGENTS.md:L3-L7
Useful? React with 👍 / 👎.
First slice of zig working end to end.
Let me know if there is anything I should change, e.g. should I make the query less specific? I have never used tree-sitter queries before