-
-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scala support #399
Scala support #399
Conversation
(putting this out there so people know it's happening, also because I can't run tests locally in WSL so this gives me CI) |
Also @pokey I'm guessing the tests are failing ("do not support plaintext") is because the version of vscode-parse-tree that's out there doesn't yet support scala? |
listing out every place where code has to change to add a new language
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good stuff! Left some inline comments. Also, it would be great to see the following tests:
- Tests for "type" where the mark is not in the type itself. I don't know scala enough to say what situations would be comprehensive, but here are a couple that jump out at me:
- Have the mark in the value / body of a function and say "clear type" to see that it clears the return type of the function
- Have the mark in the name of a function call argument and say "clear type" to see that it clears the type of the function call argument
- Similarly, tests for "value" where the mark is not in the value itself, eg it's in the name / type of the argument or assignment
- A check for "condition" where the mark is not in the condition itself
- "take round" with a document like so:
"(hello)"
, and cursor in the middle ofhello
to check that the text fragment extractor is working
// Pulled from the complete list that isn't implemented above | ||
|
||
// collectionItem: "???" | ||
// collectionKey: "???", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you want to support "AL" -> "Alabama"
expressions? For these, "key" would be "AL"
, "value" would be "Alabama"
, and "item" would be the pair.
collectionItem
is also supposed to include list elements, tho since those are just function calls you would basically use an argument matcher and check that the parent is a List
. For fancy stuff like this, you might check out the Clojure implementation. That one took tons of custom matchers to get working
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd have to write something custom for that: ("AL" -> "Alabama")
comes out as:
arguments: (arguments [1, 19] - [1, 38]
(infix_expression [1, 20] - [1, 37]
left: (string [1, 20] - [1, 24])
operator: (operator_identifier [1, 25] - [1, 27])
right: (string [1, 28] - [1, 37]))))))))
So I'd need to take the left or right of an operator, when that operator === '->'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks like it should be doable, but a bit custom. I think I'd be ok with shipping without it for now but filing an issue to capture all the missing things. I believe cursorless will generate a helpful message so that users can just see it hasn't been implemented yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw I'd capture your example here into the follow-up issue for remaining scala features; it's quite helpful
// regularExpression: "???", | ||
|
||
// attribute: "???", | ||
// xmlElement: "???", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't scala support inline html elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does (well, XML), but it's deprecated and will be dropped in 3.x, to be replaced with string formatting (I think, I don't know much about 3.x). I'm not sure how much people use it. The tree sitter implemention also errors with it.
Testing when the mark is on the name of the value.
For marks which aren't the condition itself
On types, given: class Example(foo: String) {
def str(bar: String): String = foo + bar
} We get:
From what you're saying it looks like we want to walk up the tree to find a sibling type, and take the parent. For example, if your cursor was in How would I write this? And how would I merge this with the leading matcher I'm already using that I obviously do not want to use in this case. |
On "take round", this does not work ("TypeError: Cannot read property 'endPosition' of undefined"). Any hints as to how to fix this? |
(value and condition work fine, and I added tests) |
Pretty sure you can just define a pattern matcher |
Yeah probably scala isn't creating child nodes for quotes. I'd try to steal Java's text fragment extractor |
You can now grab types when the mark is not in the type itself
OK I added code / tests for types, and used the hacked text fragment extractor and it looks to work correctly |
I'm not sure we can ever truly detect these. At least in the current tree sitter implementation they are eg just infix expressions, just like lots of other things
For when the mark is not directly on the type
Raised #434 to talk about / track pattern matching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! 👏👏
// Cursorless terminology not yet supported in this Scala implementation | ||
/* | ||
lists and maps basic definition are just function calls to constructors, eg List(1,2,3,4) | ||
These types are also basically arbitrary, so we can't really hard-code them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we hard-code them? Not saying you need to do it in this PR, but this comment implies that we can't do it, whereas I thought we were just punting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are just classes, not a syntax / language construct like []
in JS or whatever. Scala comes with a bunch of them but people will also use other random libraries as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Almost feels like we need LSP support for this one? Let's just call it out in the missing language features issue and discuss there
// Pulled from the complete list that isn't implemented above | ||
|
||
// collectionItem: "???" | ||
// collectionKey: "???", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw I'd capture your example here into the follow-up issue for remaining scala features; it's quite helpful
Basic Scala support. Tested with Scala 2.x syntax, idk enough about 3.x to know how it fares.
Relies on: cursorless-dev/vscode-parse-tree#16