Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ckipp01
Copy link
Collaborator

@ckipp01 ckipp01 commented Mar 21, 2024

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:

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?

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?
@alexarchambault
Copy link
Member

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
Copy link
Member

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

@carlosedp
Copy link
Contributor

carlosedp commented Mar 21, 2024

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
Copy link
Contributor

carlosedp commented Mar 22, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants