Add `default`, as a shortcut for `alias default` #106

dangdennis · 2019-05-30T15:31:25Z

Fixes #73

Hey Schniz, this is the draft PR for the default command. Is this something you still want?

How do I actually run your feature tests? Below are my attempts:

feature_tests/run.sh => "No binary supplied!"
feature_tests/aliases/run.sh => "feature_tests/aliases/run.sh: Permission denied"

dangdennis · 2019-05-30T15:32:24Z

executable/FnmApp.re

@@ -3,6 +3,7 @@ let version = Fnm.Fnm__Package.version;
 module Commands = {
  let use = (version, quiet) => Lwt_main.run(Use.run(~version, ~quiet));
  let alias = (version, name) => Lwt_main.run(Alias.run(~name, ~version));
+  let default = (version, name) => Lwt_main.run(Alias.run(~name, ~version));


The shorter way would've been to set ~name="default" but that doesn't look or feel right.

I totally understand your take on this, but considering how hard is it for a new contributor (and new Reason user) to understand Cmdliner, I think it's better to set the default magic string here and not in the Cmdliner bloat. This module (Commands) is easy to read and pretty much sums everything up.

And it's not that we don't have the magic default string here, it is just written in the weird land of Cmdliner, where it's harder to follow on things 😜

The other approach was to remove the default function completely because it is exactly like the alias command, and use the const("default") you have used into Commands.alias.

However, I think your choice of making a new command looks much nicer. Every command exposes a function in the Commands module. Cmdliner is just something we have to use in order to make good CLIs, but it's just an adapter 😄

What do you think?

After much thinking and mulling in what you said, I agree that the magic string in the Commands module is very suitable.

But having each Commands expose a new command with its own function is really intuitive. It also means we can customize CLI helper comments and whatnot for each command without having to overlap, say, alias and default.

:D This is fun.

dangdennis · 2019-05-30T15:33:15Z

executable/FnmApp.re

+  let sdocs = Manpage.s_common_options;
+
+  let selectedVersion = {
+    let doc = "The version to be aliased as default";


I can't believe you started learning Reason and Ocaml late 2018 from your given articles. There's a lot to take in, especially just trying to understand a library like lwt and cmdliner, on top of setting up the build system.

I normally don't try to understand 100% of the things. This is why I use pesy here instead of pure Dune, but I also used Dune in another project where I tried to learn native OCaml/Reason.

Lwt feels a lot like JS promises, and I read a lot about functional programming and these monadic interfaces for a couple of years. This is a very big thing to learn but the ppx_lwt makes it easier, just like async await in JS

Cmdliner is very hard and I just read and copied from the docs until it worked! 😜 I read on the Reason Discord that there's a ppx_deriving_cmdliner which is interesting

Ah thanks for the quick summary. Reread the lwt docs and they clearly make sense now.

And I finally understand what ppx are now too. They're like decorators in JS, or method attributes in C#. LOL.

Most of them are like decorators or method attributes, but they work at compile time, unlike JS (not sure about C# though) - including type errors, type inference and autocomplete! ❤️

Schniz · 2019-05-31T13:49:43Z

How do I actually run your feature tests?

This is totally my bad. The way to run them is by supplying the fnm binary, which is like:

esy build
./feature_tests/run.sh _build/default/executable/FnmApp.exe

Schniz · 2019-05-31T13:59:30Z

executable/FnmApp.re

@@ -3,6 +3,7 @@ let version = Fnm.Fnm__Package.version;
 module Commands = {
  let use = (version, quiet) => Lwt_main.run(Use.run(~version, ~quiet));
  let alias = (version, name) => Lwt_main.run(Alias.run(~name, ~version));
+  let default = (version, name) => Lwt_main.run(Alias.run(~name, ~version));


I totally understand your take on this, but considering how hard is it for a new contributor (and new Reason user) to understand Cmdliner, I think it's better to set the default magic string here and not in the Cmdliner bloat. This module (Commands) is easy to read and pretty much sums everything up.

And it's not that we don't have the magic default string here, it is just written in the weird land of Cmdliner, where it's harder to follow on things 😜

The other approach was to remove the default function completely because it is exactly like the alias command, and use the const("default") you have used into Commands.alias.

However, I think your choice of making a new command looks much nicer. Every command exposes a function in the Commands module. Cmdliner is just something we have to use in order to make good CLIs, but it's just an adapter 😄

What do you think?

Schniz · 2019-05-31T13:59:59Z

feature_tests/aliases/run.sh


 VERSIONS_INSTALLED=$(fnm ls)

 echo "$VERSIONS_INSTALLED" | grep 8.11.3 | grep oldie
 echo "$VERSIONS_INSTALLED" | grep 6.11.3 | grep older
 echo "$VERSIONS_INSTALLED" | grep 6.11.3 | grep default


Schniz · 2019-05-31T14:29:18Z

Thanks for this PR! I'm sure people will be happy with it 😄
Awesome work understanding how to work with Cmdliner, by the way! 🌮 🌵

dangdennis · 2019-06-01T01:28:16Z

Darn, if we use the magic string default, the Commands.default breaks into a second line. Totally superfluous issue. Is this a prettier or eslint setting you set up somewhere?

Schniz · 2019-06-01T20:39:39Z

Is this a prettier or eslint setting you set up somewhere?

I use refmt which works like Prettier, or, actually, the other way around 😉

fnm/package.json

Lines 122 to 125 in cd0cb1a

    
           "*.re": [ 
        
             "esy refmt --in-place", 
        
             "git add" 
        
           ],

The linebreak doesn't bother me 😄

Schniz

amazing! 🌮
Thanks 😄

dangdennis added 3 commits May 30, 2019 11:23

New command for quick alias defaults

f4da21f

Add docs for alias + default

04694e2

Update alias test with default usage

e52f172

dangdennis commented May 30, 2019

View reviewed changes

dangdennis marked this pull request as ready for review May 31, 2019 01:40

Schniz reviewed May 31, 2019

View reviewed changes

Schniz added the PR: New Feature A new feature was added label May 31, 2019

dangdennis added 2 commits May 31, 2019 21:20

Actually use Command.default

5299cfe

Move Commands.default into one line

44514d1

Schniz approved these changes Jun 1, 2019

View reviewed changes

Schniz changed the title ~~Feat default alias~~ Add default, as a shortcut for alias default Jun 1, 2019

Schniz merged commit 430541a into Schniz:master Jun 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add `default`, as a shortcut for `alias default` #106

Add `default`, as a shortcut for `alias default` #106

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add default, as a shortcut for alias default #106

Add default, as a shortcut for alias default #106

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `default`, as a shortcut for `alias default` #106

Add `default`, as a shortcut for `alias default` #106