Switch to optparse

[deemp]

@f-f, this PR addresses #977

Description of the change

I used optparse instead of argparse for building spaghetto CLI.

optparse supports completions for bash, zsh, fish.

My tools were purescript 0.15.10 and spago 0.93.9.

I followed the Developing instructions and built an executable in the spaghetto directory.

spago bundle -p spago-bin
cd bin
ln -s bundle.js spaghetto

Next, I manually tested completions

source <(./spaghetto --bash-completion-script ./spaghetto)
./spaghetto <TAB>
build     bundle    fetch     -h        --help    init      install   ls        publish   registry  repl      run       sources   test

Finally, I ran tests with

spago test

11/13 tests were successful.

Warning

• I added metavars like ARGS in some places. They can be renamed/removed when necessary
• As there were many flags, I may have made some typos

[f-f]

We'll need a spago --version flag to return the current version number, which is what the BuildInfo.currentSpagoVersion returns

[deemp]

I decided to use a command instead of a flag for printing the version.

459b17c

This command doesn't trigger printing the help message when prefShowHelpOnError is set to true.

I considered infoOption, but this option always fails. So, it prints both a version and a help message.

[f-f]

We'll need a flag for the version, rather than a command - this has come up before, see #608

I'd basically like to mirror the behaviour of purs:

$ purs --version
0.15.9

..so it would we great if we can avoid printing the help message

[deemp]

I added a --version flag. After spago bundle -p spago-bin:

$ bin/bundle.js --help
Usage: bundle.js ([COMMAND] | [-v|--version])
  PureScript package manager and build tool

Available options:
  -h,--help                Show this help text
  -v,--version             Show the current version


Available commands:
  init                     Initialise a new project
...

$ bin/bundle.js --version
0.93.9

[deemp]

Should I remove the version command?

[f-f]

I added a --version flag

Looks great!

Should I remove the version command?

Yes please! We shouldn't add one unless it's necessary.

[deemp]

Removed in 87ccdf6

[deemp]

@f-f, please, review

[f-f]

@deemp I was on holiday and mostly offline, will have a look now

[f-f]

I'm sorting out CI so that we can merge - we still have the Haskell CI, so the tests for the new spago are not running at all yet

[f-f]

We have PureScript CI now! Could you rebase on the latest commit? I had to shuffle files around unfortunately, so it might be easier to just copy the patch to a new branch instead of trying to merge.

[f-f]

@deemp did you get any chance to look at this? anything I could help with?

[deemp]

@f-f, rebased and built 👌

[deemp]

@f-f, why does the -p flag work with the top-level command? Where is it defined?

spago/.github/workflows/build.yml

Line 76 in 5300c57

run: ./bin/index.dev.js -p spago-bin

UPD: lol, it shouldn't work with the top-level command
https://github.com/purescript/spago/actions/runs/6285580917/job/17068112404#step:12:6
But the error is ignored in this workflow.

[f-f]

But the error is ignored in this workflow.

It's a good thing this fails explicitly now 🙂

[f-f]

Thanks for rebasing @deemp! This looks great now - I left a few comments, mostly for things that slipped in the merge, but it's otherwise good to go

[f-f]

Ah, CI doesn't seem to be happy - we got a few of nonexistent options being used in the test (which should be easy to fix), but the bulk of the issue is - I think at least - the fact that we are using records to collect flags and arguments: this was ok when using argparse, because we used anyNotFlag. The new argument collection does not, so I suppose we should switch to applicative-do for collecting the flags first, and then collect any remaining arguments.

[f-f]

Still a few tests failing - @deemp you can run tests locally with spago test to iterate more quickly (get the latest spago with npm install -g spago@next)

[f-f]

@deemp amazing - feel free to comment out the remaining failing test, the -f flag was never implemented so it was passing just by accident 😄

[f-f]

Ah nice, this is finally green - merging.

Thanks a lot @deemp for the patch! 👏

[deemp]

@f-f, thanks for your guidance!