skip installation if the version is already installed

[kentac55]

1. When install specify version from fnm ls-remote, the version which has been installed contains ANSI codes and fnm install raise internal error.

i reverted it.

$ fnm install $(fnm ls-remote | grep -E "\* v[0-9]?[0,2,4,6,8]" | tail -n 1 | sed -r -e "s/\* //g")

Looking for node v10.15.1 for linux x64
fnm: internal error, uncaught exception:
     (Failure hd)
     Raised at file "pervasives.ml", line 32, characters 17-33
     Called from file "list.ml" (inlined), line 27, characters 10-23
     Called from file "library/Http.re", line 36, characters 25-41
     Called from file "library/Http.re", line 43, characters 2-27
     Called from file "src/core/lwt.ml", line 1929, characters 23-26
     Re-raised at file "library/Http.re", line 41, characters 2-133
     Re-raised at file "library/Versions.re", line 80, characters 2-580
     Re-raised at file "executable/Install.re", line 45, characters 2-942
     Re-raised at file "src/core/lwt.ml", line 2987, characters 20-29
     Called from file "src/unix/lwt_main.ml", line 26, characters 8-18
     Called from file "cmdliner_term.ml", line 25, characters 19-24
     Called from file "cmdliner.ml", line 116, characters 32-39

2. skip to install the version which has been already installed

[Schniz]

Woah! @kentac55! thanks for the contribution 😸
This is a very good feature and I think we can polish it a bit and then merge it because it is a very good idea.

Another idea, that may not fit for right now - is adding a --reinstall flag

About ANSI, I totally agree with you. Kinda annoying that we force colors all the time, but I think removing the ANSI codes shouldn't be on our input, but our output. Maybe we should have something like Chalk's supports-color? I'm opening a new issue for that matter, so let's focus this PR on the "already installed" part

[Schniz]

Thanks @kentac55! it looks much better and I hope #28 will help us with passing results between calls.

Can you please rename the function so it would be throwIfMissing or something that would match its type signature of Lwt.t(unit)?

[kentac55]

@Schniz Thank you for your reviewing! And sorry for my confusing(it's my first time to write reasonml(and ocaml) 😵 and I misread your review... 😰 )

[Schniz]

Hey, everything is good! I'm trying not to nitpick so if I'm too annoying just tell me 😄

I have added some comments, and I'm sorry I haven't addressed this beforehand. I guess the code will be cleaner after some slight changes - I'd like to know what you think about it too

Thanks again, my friend! I'm thankful for your contribution and welcome to our community 🌮

[Schniz]

If we would fold it to an empty array, we could push down the knowledge of Lwt and take out one indentation level:

let installed = getInstalledVersions()
|> Result.fold(_ => [||], xs => xs) // would return an empty array for errors
|> Array.exists(x => Local.(x.name == versionName))

if (installed) {
  Lwt.fail(Already_installed(versionName));
} else {
  Lwt.return();
}

[Schniz]

that being said, I'm really not sure we need to even care about Lwt here. We can stick to isVersionInstalled like you mentioned before that, and leave the logic of throwing an error to the executable itself (which also handles the error):

// library/Versions.re

let isVersionInstalled = versionName =>
  getInstalledVersions()
  |> Result.fold(_ => [||], xs => xs) // would return an empty array for errors
  |> Array.exists(x => Local.(x.name == versionName));

// executable/Install.re

let%lwt _ = if (Versions.isVersionInstalled(someVersion)) {
  Lwt.fail(Already_installed(someVersion));
} else {
  Lwt.return();
}

[Schniz]

Hey @kentac55, what's up? I want to merge this and #30, and I don't want to create conflicts for your branch by merging the other 😄

[theandrewlane]

@Schniz /@kentac55 pleeaaaseee get this merged in soon ♥️

[Schniz]

Now that I think about it, in #30 I'm making readdir async (with Lwt), therefore this function would be async as well. I'm merging 😄

[Schniz]

@kentac55, thanks for all your hard work 😄 🏆 🌮

[kentac55]

@Schniz Sorry for late response. I was too busy to response this PR in few days. Thanks for fix and merge it and taught reasonml for me.