-
Notifications
You must be signed in to change notification settings - Fork 17
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
refactor: start typing all the Strings #247
Conversation
This is sort of a proposal. Following the comment in coursier#243 (comment) by @carlosedp I was going to try and write some docs on the actual structure of the index, but right away hit on the fact that it's just this: ``` final case class Index(map: Map[String, Map[String, Map[String, Map[String, String]]]]) { ``` It's pretty impossible to know what these `String`s all are so I figured maybe it'd be good to start just typing this out a bit more. I used an opaque type for `Os` here in hopes that it'd make things clearer a bit throughout the code, and figured we could do that for all the `String`s in the `Index`, but wanted to see what it would be like to at least do one of them first. Thoughts on this?
That looks great! We could even go further than that, and enforce only pre-defined |
Feel free to do the same for the architecture too. And a dedicated type for JVM id would be nice too, with no pre-defined values in the companion for this one, I guess |
Oh, sorry about not giving a feedback yet, I had a ton of meetings today and just got home. This would avoid mistakes like I did when some JVMs ended on a macos index (that shouldn't exist). |
I also though about having something like: enum OSs:
case linux, `linux-musl`, darwin, windows, aix, alpine, solaris
// Map the JVM index to the OS
object OSs:
def map(os: String): OSs = os match
case "linux" => OSs.linux
case "darwin" => OSs.darwin
case "macos" => OSs.darwin
case "windows" => OSs.windows
case "aix" => OSs.aix
case "alpine" => OSs.alpine
case "solaris" => OSs.solaris
case "linux-musl" => OSs.`linux-musl`
case _ => throw IllegalArgumentException(s"Unknown OS: $os")
enum Archs:
case amd64, arm64, x86, ppc64le, s390x, ppc64
object Archs:
// Map the JVM index to the Arch
def map(arch: String): Archs = arch match
case "amd64" => Archs.amd64
case "x64" => Archs.amd64
case "arm64" => Archs.arm64
case "aarch64" => Archs.arm64
case "x86" => Archs.x86
case "ppc64le" => Archs.ppc64le
case "s390x" => Archs.s390x
case "ppc64" => Archs.ppc64
case _ => throw IllegalArgumentException(s"Unknown Arch: $arch")
enum Exts:
case zip, tgz
object Exts:
// Map the JVM index to the Ext
def map(ext: String): Exts = ext match
case "zip" => Exts.zip
case "tgz" => Exts.tgz
case "tar.gz" => Exts.tgz
case _ => throw IllegalArgumentException(s"Unknown Ext: $ext") To avoid all mapping logic in the index builders too. This would avoid mistakes and reduce the amount of code. Also the objects could also generate the URL outputs and etc... |
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.
Merging what's already been done, thanks @ckipp01
This is sort of a proposal. Following the comment in
#243 (comment)
by @carlosedp I was going to try and write some docs on the actual
structure of the index, but right away hit on the fact that it's just
this:
It's pretty impossible to know what these
String
s all are so I figuredmaybe it'd be good to start just typing this out a bit more. I used an
opaque type for
Os
here in hopes that it'd make things clearer a bitthroughout the code, and figured we could do that for all the
String
sin the
Index
, but wanted to see what it would be like to at least doone of them first. Thoughts on this?