feat(repl): support the toolkit directive and :toolkit command - #26788
feat(repl): support the toolkit directive and :toolkit command#26788warcholjakub wants to merge 6 commits into
Conversation
c2adf51 to
1d0737e
Compare
SolalPirelli
left a comment
There was a problem hiding this comment.
The underlying logic looks sound but toolkitCoordinates and the Toolkit trait are rather convoluted. Do we really expect to support weird new toolkits whose matchesFlavor isn't set membership? Could we use a Map[String, String] to map aliases instead?
| val rawVersion = tokens.lastOption.filter(_.nonEmpty).getOrElse("default") | ||
| val flavor = tokens.dropRight(1).headOption.getOrElse("scala") |
There was a problem hiding this comment.
I'm rather confused by what's going on here. Is it OK to have more than 2 tokens? Is it ever OK to have an empty token? Do we really need to support xyz: ? It seems like this should match on Nil, version :: Nil, and flavor :: version :: Nil and reject everything else, as well as reject empty versions and flavors?
There was a problem hiding this comment.
Yeah, that's a fair point, but the logic is pretty much copied from the scala-cli. Although I'm not sure what's a better option: to do this as scala-cli does (not perfect), or deviate from the original behaviour and handle it better(?).
There was a problem hiding this comment.
Discussed it offline with @Gedochao, and it's indeed not intended. Also applies to the second comment (: and :: shouldn't be supported). Anyhow, I'll fix it.
| "com.example:1.2.3" -> ("com.example", "1.2.3"), | ||
| "com.example:latest" -> ("com.example", "latest.release"), | ||
| ":" -> ("org.scala-lang", "0.9.2"), | ||
| "::" -> ("org.scala-lang", "0.9.2") |
There was a problem hiding this comment.
This is a supported use case? 😱
There was a problem hiding this comment.
Technically it doesn't work in scala-cli. But it's not really because it's unsupported per se (at least that's what I think). Scala-cli uses tokens.last and it throws an error when trying to use .last on an EmptyArray. I used lastOption and it works, but at a cost of allowing this particular use case. I agree that's a lil bit cursed, and I'm open to suggestions on what to do with that.
Fair, I got a little carried away 😅. EDIT: Addressed in 4362c99 |
6808a4e to
df61878
Compare
Fixes #26744
Adds
//> using toolkitdirective and:toolkitcommand.Have you relied on LLM-based tools in this contribution?
Yes, and I checked the output by testing manually with bin/replQ
How was the solution tested?
New automated tests (including the issue's reproducer, if applicable)