-
Notifications
You must be signed in to change notification settings - Fork 12
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
[proposal] ScalacOption class based on types #18
base: main
Are you sure you want to change the base?
Conversation
Ok.. But it won't work for options like |
* It deliberately made not returning any failure type like `ParseFailure`, becase | ||
* `ScalacOption.Select` is responsible for that. | ||
*/ | ||
def parse(rawValue: String): Option[A] |
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.
Right, I think it should be rawValue: Seq[String]
new Select[A] { | ||
type Out = A | ||
|
||
def parse(rawValues: List[String]): Option[A] = { |
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 imagine this method will need to delimit the scalac options without knowing all of them. One easy algorithm is to treat every -something
as the beginning of a new option+args.
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 would say that a more reliable (and performant, actually) way could be pre-parsing all the options at once, and then just picking them by one from the collection of parsed options upon a user request. But it would require a global "registry" of parsers. Which is not that bad, actually, but makes the approach different from how Headers are treated.
The root cause of the difference is kinda "fundamental" – in regards to headers there're RFCs that clearly define how to separate various headers from each other without digging into their contents. But for CLI options there's no such a clear way of separating 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.
But it would require a global "registry" of parsers.
I agree it would be much more reliable, but I would very much like to avoid this for fear this library will completely stop working if it encounters an unknown scalac option. It must be able to cleanly pass-through anything it cannot recognize.
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.
Right, we definitely should try to make the parser more lenient.
But that should be a fall-back mode, I think. I.e. if the parser cannot recognize something it can try to pass-through assuming that everything not starting with a dash is an argument rather than an option.
It would be a good trade-off between reliability and tolerance.
a8dcb52
to
3ea4f8b
Compare
3ea4f8b
to
145dc24
Compare
@satorg so where did we leave this one? 😅 FTR no pressure or anything, just trying to figure if/how things are blocked. We do have to do sbt-tl 0.5.x sometime, but believe me I am in no rush 😬 |
Yeah, personally I would really love to continue on this... Just got distracted a little bit with some other less pleasant but more urgent stuff, unfortunately. Hopefully, I will sort it out soon and be able to return to more pleasant things like this one :) |
I know the feeling :) thanks for the update and hope you have a nice holiday!
Ha, that's fantastic. I am not nearly so thrilled about scalac options 😂 so I'm glad we found the right person for the job 😉 |
I started sketching out some ideas according to our conversations with @armanbilge and @DavidGregory084.
See also #2.
Basically, it is very similar to how
org.http4s.Header
works:Then in build.sbt:
Still requires a lot of work and refinements. And tests of course :)
Note: I put all the new classes into a
proposal
sub-package just to do not interfere with the existing code.