jvm-index icon indicating copy to clipboard operation
jvm-index copied to clipboard

refactor: start typing all the Strings

Open ckipp01 opened this issue 1 year ago • 4 comments

This is sort of a proposal. Following the comment in https://github.com/coursier/jvm-index/pull/243#issuecomment-2007605083 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 Strings 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 Strings in the Index, but wanted to see what it would be like to at least do one of them first. Thoughts on this?

ckipp01 avatar Mar 21 '24 13:03 ckipp01

That looks great! We could even go further than that, and enforce only pre-defined Os values, with vals in the Os companion, making the constructor private, and enforcing creating an Os from a dynamic value in a Os.from method say, that would just throw on un-recognized values

alexarchambault avatar Mar 21 '24 22:03 alexarchambault

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

alexarchambault avatar Mar 21 '24 22:03 alexarchambault

Oh, sorry about not giving a feedback yet, I had a ton of meetings today and just got home. I'll take a look tomorrow morning but think having the fields validated as enums or something like that would be great.

This would avoid mistakes like I did when some JVMs ended on a macos index (that shouldn't exist).

carlosedp avatar Mar 21 '24 23:03 carlosedp

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... What do you think? Too much hassle to keep this updated to all required combinations from indexes?

carlosedp avatar Mar 22 '24 13:03 carlosedp