skip installation if the version is already installed #27

kentac55 · 2019-02-11T13:53:57Z

~~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

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

library/Versions.re

This reverts commit 9e5b27f.

Schniz · 2019-02-12T13:14:22Z

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 · 2019-02-12T14:11:26Z

@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 🌮

library/Versions.re

Schniz · 2019-02-12T15:42:13Z

library/Versions.re

+let throwIfInstalled = versionName => {
+  getInstalledVersions()
+  |> Result.fold(
+       _ => Lwt.return(),


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(); }

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 · 2019-02-14T07:03:46Z

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 · 2019-02-14T07:59:44Z

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

Schniz · 2019-02-14T08:12:02Z

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 · 2019-02-14T08:12:23Z

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

kentac55 · 2019-02-15T13:01:51Z

@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.

kentac55 added 2 commits February 11, 2019 22:48

remove ansi codes from provided version string

9e5b27f

add check func whether the specify version is already installed or not

c8079bf

kentac55 changed the title ~~Patch 1~~ skip installation if the version is already installed Feb 11, 2019

Schniz requested changes Feb 11, 2019

View reviewed changes

library/Versions.re Outdated Show resolved Hide resolved

Schniz mentioned this pull request Feb 11, 2019

Strip colors automatically #28

Closed

Revert "remove ansi codes from provided version string"

80c5c8b

This reverts commit 9e5b27f.

apply code review(rename check function)

83a52e0

Schniz reviewed Feb 12, 2019

View reviewed changes

library/Versions.re Outdated Show resolved Hide resolved

library/Versions.re Outdated Show resolved Hide resolved

library/Versions.re Outdated Show resolved Hide resolved

library/Versions.re Outdated Show resolved Hide resolved

library/Versions.re Outdated Show resolved Hide resolved

kentac55 added 4 commits February 12, 2019 23:56

run esy fmt

07f7457

move short path to the top of the switch case

2103111

use Result.fold instead of switch case

5700c46

use Array.exists instead of List.exists

7f92279

Schniz reviewed Feb 12, 2019

View reviewed changes

This was referenced Feb 13, 2019

Allow downloading versions from a Node.js mirror #32

Closed

fnm install fails if target Node version already exists #34

Closed

Schniz merged commit a1e9266 into Schniz:master Feb 14, 2019

Schniz added the PR: New Feature A new feature was added label Feb 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip installation if the version is already installed #27

skip installation if the version is already installed #27

kentac55 commented Feb 11, 2019 •

edited

Loading

Schniz left a comment

Schniz commented Feb 12, 2019

kentac55 commented Feb 12, 2019 •

edited

Loading

Schniz left a comment

Schniz Feb 12, 2019

Schniz Feb 12, 2019

Schniz commented Feb 14, 2019

theandrewlane commented Feb 14, 2019 •

edited

Loading

Schniz commented Feb 14, 2019

Schniz commented Feb 14, 2019

kentac55 commented Feb 15, 2019 •

edited

Loading

skip installation if the version is already installed #27

skip installation if the version is already installed #27

Conversation

kentac55 commented Feb 11, 2019 • edited Loading

Schniz left a comment

Choose a reason for hiding this comment

Schniz commented Feb 12, 2019

kentac55 commented Feb 12, 2019 • edited Loading

Schniz left a comment

Choose a reason for hiding this comment

Schniz Feb 12, 2019

Choose a reason for hiding this comment

Schniz Feb 12, 2019

Choose a reason for hiding this comment

Schniz commented Feb 14, 2019

theandrewlane commented Feb 14, 2019 • edited Loading

Schniz commented Feb 14, 2019

Schniz commented Feb 14, 2019

kentac55 commented Feb 15, 2019 • edited Loading

kentac55 commented Feb 11, 2019 •

edited

Loading

kentac55 commented Feb 12, 2019 •

edited

Loading

theandrewlane commented Feb 14, 2019 •

edited

Loading

kentac55 commented Feb 15, 2019 •

edited

Loading