Added support for Shell script language. - #2058
Conversation
pokey
left a comment
There was a problem hiding this comment.
Once again fantastic work. Left a few minor comments
|
Oh also fwiw the infrastructure to formalize those test facets is now in place. Feel free to try it out!
|
307d7ea to
f235bda
Compare
|
All issues from initial review pass are fixed. I have support for a few node types that are dependent on an updated tree-sitter-bash (vscode-parse-tree PR #77). I've also fixed lots of other quirks and edge cases I ran into. I added the initial scope facet files, but running the |
|
I switched to tip of bash branch, and noticed some tests breaking. I'm not sure if that was the cause, but I fixed them. Along the way I cleaned up a bit and added / updated facet tests. The auto facet update worked fine for me fwiw See my fixes; haven't looked at all the code, but possible some of the techniques there apply elsewhere? But if not limk and I'll continue reviewing |
How do I actually run this for a subset? There is very little documentation for the regular expressions, and the example folders don't seem to actually exist. |
See https://www.cursorless.org/docs/contributing/#running-a-subset-of-tests Thought the regex was documented somewhere. It’s just the one supported by mocha cli. A PR to add a link to that to those docs would be much appreciated (pasted from slack form posterity)
Sorry, not sure I follow. Which example folders? |
|
My problem was the regex. I realized after I ran the non subset and it worked. My regex was set to languages/shellscript because that's what I had been doing for the test cases (not sure why I prefixed languages/ in the first place), and of course scope doesn't have the languages/ subdir. Fwiw, one error I get when running tests (though doesn't impact the scopes updating): Not sure if this is another issue with TMPDIR style stuff or what. Didn't get time to dig into it yet. |
|
Is your cursorless talon up to date? |
|
lmk if you want me to review the code or if you wanted to do another pass first |
|
I'll take a look at it tomorrow to see if I spot any candidates for using removal, and also add the fixtures I've been working on. |
d21b706 to
59a9341
Compare
|
Trying to update this in light of the conflict from getLanguageScopeSupport being deleted, and seeing new pre-commit errors I've not run into before: Will look into it, but just throwing it out here in case you happen to know what to do. |
|
The tailwind bug is from commit de37ebf. This adds an external pnpm package dependency that pre-commit itself won't install, so pre-commit install is no longer enough. Nixos doesn't have a prettier-plugin-tailwindcss npm package, so atm I have to manually pnpm install -w -D prettier-plugin-tailwindcss, which then changes the pnpm-lock.yaml file lol. :/ I will create a nix package for prettier-plugin-tailwindcss and get it upstreamed, but in the meantime I'll have to use a bit of a hack to get that to work I guess (will just install the package in the dev shell and then git restore the lock file..) The second bug seems to be related to me recently changing the flake.nix file to use corepack per auscompgeek suggestion. Reverting it back to using hardcoded nodejs 18 makes it go away, so I will revert that latest commit in I guess, as using pre-commit wasn't something I had tested. |
|
Looks like a bunch of tests are breaking related to @dummy and other things which I vaguely recall fixing a long time ago, so not sure what's up, but will investigate. |
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: eedaf0a3d0
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1de69cc8e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd0947cf1d
ℹ️ 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".
What
Adds support for some next gen scopes to
shellscriptprogramming language.This is something I did recently while working on some bash scripts, but haven't had time to finish it off yet.
Checklist
"change"/"clear"instead of"take"for selection tests to make recorded tests easier to read"chuck"instead of"change"to test removal behaviour when it's interesting, especially:"chuck arg"with single argument in list"chuck arg"with multiple arguments in list"chuck item"with single argument in list"chuck item"with multiple arguments in list@textFragmentcaptures. Usually you want to put these on comment and string nodes. This enables"take round"to work within comments and strings."change round"inside a string, eg"hello (there)""type"both for type annotations (egfoo: string) and declarations (eginterface Foo {}) (and added tests for this behaviour 😊)"item"both for map pairs and list entries (with tests of course)Scope Support
Legend: ✓ Supported, ✗ Not supported, _ Not applicable
list@listinside list@list.interiormap@mapinside map@map.interiorkey@collectionKeyfunk@namedFunctioninside funk@namedFunction.interiorfunk name@functionNamelambda@anonymousFunctioninside lambda@anonymousFunction.interiorname@namevalue@valuevalue@valuevalue@valuestate@statementif state@ifStatementcondition@conditioncondition@conditioncondition@conditioncondition@conditioncondition@conditioncondition@conditionbranch@branchinside branch@branch.interiorcomment@commentcomment@commentinside comment@comment.interiorstring@stringstring@string@textFragmentcall@functionCallcallee@functionCalleearg@argumentOrParameterarg@argumentOrParameterarg@argumentOrParameterclass@classinside class@class.interiorclass name@classNametype@typeregex@regex