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

Add support for .node-version files #42

Merged
merged 5 commits into from Feb 18, 2019

Conversation

Dean177
Copy link
Contributor

@Dean177 Dean177 commented Feb 16, 2019

I attempted to add support for .node-version files.

I'm brand new to the reason ecosystem so have absolutely no idea what I am doing.

The files in the esy.lock directory seem to have changed quite substantially, have I done something wrong to cause this?

Resolves #8

Copy link
Owner

@Schniz Schniz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very very very good! Thanks!
Take a look at what I've commented - what do you think?


let getVersion = () => {
let%lwt cwd = Lwt_unix.getcwd();
try%lwt (versionString(Filename.concat(cwd, ".node-version"))) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if versionString would catch the errors and provide option(string), that means that Some("file contents") or None when file not found?
then, this code would be easier to follow and Reason's amazing pattern matching will help us:

let%lwt nodeVersion = Filename.concat(cwd, ".node-version") |> versionString
and nvmrc = Filename.concat(cwd, ".nvmrc") |> versionString;

switch (nodeVersion, nvmrc) {
| (None, None) => Lwt.fail(Version_Not_Provided)
| (Some(_), Some(_)) => Lwt.failwith("You have both .node-version and .nvmrc!")
| (Some(version), None)
| (None, Some(version)) => Lwt.return(version)
};

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or even:

let%lwt nodeVersion = Filename.concat(cwd, ".node-version") |> versionString
and nvmrc = Filename.concat(cwd, ".nvmrc") |> versionString;

switch (nodeVersion, nvmrc) {
| (None, None) => Lwt.fail(Version_Not_Provided)
| (Some(version), Some(_))
| (Some(version), None)
| (None, Some(version)) => Lwt.return(version)
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea and thanks for the feedback. I think in the case where a user has both a .node-version and an .nvmrc we can be permissive if they contain matching versions (since they might just want to support multiple clients).

Whats the difference between:

let%lwt nodeVersion = Filename.concat(cwd, ".node-version") |> versionString
and nvmrc = Filename.concat(cwd, ".nvmrc") |> versionString;

and

let%lwt nodeVersion = Filename.concat(cwd, ".node-version") |> versionString;
let%lwt nvmrc = Filename.concat(cwd, ".nvmrc") |> versionString;

The docs mention parallel binding: https://ocsigen.org/lwt/3.2.1/api/Ppx_lwt
Does this mean they will actually be run in parallel?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it’s like Promise.all!

log(
~quiet,
<Pastel color=Pastel.Red>
"No .nvmrc was found in the current directory. Please provide a version number."
"No .nvmrc or .node-version file was found in the current directory. Please provide a version number."
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@Schniz
Copy link
Owner

Schniz commented Feb 17, 2019

I'm brand new to the reason ecosystem so have absolutely no idea what I am doing.

May I just add, that it sure doesn't look this way.
Looks very very good 😄

Succeed if they match, fail if they differ.
Copy link
Owner

@Schniz Schniz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much much cleaner! 🤩
What do you think about taking the conditional into the pattern matching clause?

switch (nodeVersion, nvmrc) {
| (None, None) => Lwt.fail(Version_Not_Provided)
| (Some(nodeVersionString), Some(nvmrcString)) =>
if (nodeVersionString === nvmrcString) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can even be reduced further using:

  | (Some(v1), Some(v2) when v1 == v2 =>

Then, no nesting in our pattern matching! Wdyt?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better; check if it isn’t equal first :)

Copy link
Owner

@Schniz Schniz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing 🏆 💯 🌮

Thanks!

@Schniz Schniz merged commit fd86d8c into Schniz:master Feb 18, 2019
@Schniz Schniz added the PR: New Feature A new feature was added label Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature A new feature was added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants