Make the completion logic easier to extend #11794
Conversation
alexarchambault
force-pushed
the
custom-completions
branch
from
acaac07 to
9441ab6
Compare
|
About the stability of the methods that are made public: as far as Ammonite is concerned, it's ok if binary and / or compatibility are broken from time to time, as long as Ammonite can still extend completion the way it's done in the additional tests. |
| // initially adapted from | ||
| // https://github.com/scala/scala/blob/decbd53f1bde4600c8ff860f30a79f028a8e431d/ | ||
| // src/reflect/scala/reflect/internal/Printers.scala#L573-L584 | ||
| else if (name == nme.CONSTRUCTOR) "this" |
There was a problem hiding this comment.
I'm not really sure if it's possible to get a completion that would require such renaming. Primary constructors are explicitly excluded while collecting completions. I have no idea why this is not done explicitly for secondary constructors but I they don't seem to be listed among other completions. BTW completing names after new doesn't seem to work currently
There was a problem hiding this comment.
That comes from the linked code. I removed it.
| else { | ||
| val decName = name.decode.toString | ||
| val hasSpecialChar = decName.exists { ch => | ||
| brackets.contains(ch) || ch.isWhitespace || isDot(ch) |
There was a problem hiding this comment.
We should also include comma, semicolon and not sure what else. I checked these in ammonite and they seem not to get backticks there as expected. Non-letter unicode characters like § don't get handled properly either. I'm wondering if we could reuse some existing logic (e.g. taken from the Scanner) to find out if something is a valid identifier name by itself or not instead of reinventing the wheel here. In case that would be too heavy in terms of performance we could first filter out the names that for sure don't require backticks and then validate the rest with the proper logic.
We should also add some dedicated tests for such corner cases
There was a problem hiding this comment.
I agree, but maybe this can be fixed later? As soon as this PR is merged, this should allow to merge com-lihaoyi/Ammonite#1150 (using dotty nighlies).
| val hasSpecialChar = decName.exists { ch => | ||
| brackets.contains(ch) || ch.isWhitespace || isDot(ch) | ||
| } | ||
| def isOperatorLike = (name.isOperatorName || decName.exists(isOperatorPart)) && |
There was a problem hiding this comment.
Am I right that this would put all simple operators like + or :: into backticks? That seems unnecessary. Also mixed Identifiers like foo_++ don't need backticks.
There was a problem hiding this comment.
It seems it doesn't for operators like ++ or ::, thanks to the condition decName.exists(isScalaLetter) right after I think. But it does for things like foo_++, yes, which is unfortunate.
I have a slightly different version of that PR, that allows not to change the current behavior in dotty, while still allowing Ammonite to do things slightly differently in that area. Would you be fine with that?
There was a problem hiding this comment.
Ok, I just pushed that (no back-tick-related changes for the repl itself).
| import org.junit.Assert._ | ||
| import org.junit.Test | ||
|
|
||
| class CompletionTests extends DottyTest: |
There was a problem hiding this comment.
I would suggest renaming this to something like CustomCompletionTest not to cause confusion with the already existing CompletionTest
| isOperatorLike || | ||
| nme.keywords(term) && term != nme.USCOREkw | ||
|
|
||
| if (needsBackTicks) s"`$decName`" |
There was a problem hiding this comment.
What would happen if a user has already typed a backtick before pressing TAB in REPL?
There was a problem hiding this comment.
It doesn't work too well... We pass both back-ticked and non-back-ticked labels to JLine, but it sometimes only keeps one or the other. In manual tests, it completes Foo. with both, but Foo.a or Foo.`a` only with the non-back-ticked (former) or back-ticked (latter).
There was a problem hiding this comment.
In the code I just pushed, the dotty repl still behaves as before (no back ticks whatsoever).
alexarchambault
force-pushed
the
custom-completions
branch
from
9441ab6 to
45fddb0
Compare
alexarchambault
force-pushed
the
custom-completions
branch
from
45fddb0 to
c3f59a5
Compare
Ref com-lihaoyi/Ammonite#1150
This PR makes the code handling completion easier to extend, so that Ammonite can rely on it and can add support for its
import $ivyand "deep" completions.In more detail, this PR:
Completion,It also adds non-regression tests, testing that the API can be extended the way Ammonite expects it, and that back-ticks are added when necessary.