Open wolfmanstout opened 2 weeks ago
Creating a draft pull request as suggested. Please let me know if you have any input on how I'm doing things so far. I'm following the same sequence as java.scm
where applicable, although it's not a perfect 1:1 mapping. The tree-sitter-kotlin implementation could definitely be improved, but I'm hoping to avoid changes there, at least for this first PR.
Kotlin does not have any list or map literals, or even array literals. Everything is just a function call, and the closest thing to a map literal is use of the "to" infix operator, like so:
val map = mapOf(1 to "x", 2 to "y", -1 to "zz")
println(map.get(1)) // {1=x, 2=y, -1=zz}
Hence, I guess I don't need to explicitly define item
but I should just test that it works with args? I could also assume that pairs constructed with that syntax are [key] to [value]
pairs.
Also, should I be supporting and testing @private.switchStatementSubject
? I guess I'll need to manually bind this to a scope name.
I think this is now ready for review. Some open questions:
Also, I noticed that "drink/pour arg" don't automatically work to add commas. How can I add support for these?
Also, I noticed that "drink/pour arg" don't automatically work to add commas. How can I add support for these?
Here is the implementation for javascript you can have a look at https://github.com/cursorless-dev/cursorless/blob/089ba0da1461dd14ea644a6f000526d920e0110a/queries/javascript.core.scm#L692-L718
Thanks @AndreasArvidsson, I added argument delimiter support.
Ok before you get too much further, it's worth having a look at our updated docs for adding a new language. In particular, we have a new way of writing tests, and we now advocate for many small PRs rather than one big PR.
Given that I added those new docs after you had already opened this PR, I won't make it a hard requirement for merging, but going forward, let's
a) not add any more scopes to this PR, and b) use the new test format for subsequent PRs
We're planning to add a migration assistant for migrating tests to the new format (see #2390), so I don't think it's worth migrating these manually
I'll have a look at this PR in the next couple of days, but figured I'd give you a chance to look at those docs in case there's anything you want to tweak before I have a look
Thanks! I don't think I will have time to make any changes to this pull request within the next couple days, so you should feel free to review.
Unfortunately it's a little late to stop adding more scopes because all the scopes have been added 😂. I'm happy to add the metadata files before submitting that describe what scopes are covered.
Thanks! I don't think I will have time to make any changes to this pull request within the next couple days, so you should feel free to review.
Sounds good; will do 😊
Unfortunately it's a little late to stop adding more scopes because all the scopes have been added 😂
Ha no worries
I'm happy to add the metadata files before submitting that describe what scopes are covered.
You'd need to migrate your recorded tests to new scope tests as well, because we have checks to ensure that all scope facets marked as supported actually have tests. So if you're not planning to manually migrate those tests (which I'm not sure I'd recommend), I'd leave out the support tables for now
Okay I won't plan to add the tables then. You can just let me know in the review if there are any other small things I should add as part of this change.
What
Adds support for the
kotlin
programming languageChecklist
"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@textFragment
captures. 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)