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

Make all wasp cli commands to use optparse-applicative #1202

Closed
wants to merge 82 commits into from
Closed

Make all wasp cli commands to use optparse-applicative #1202

wants to merge 82 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 15, 2023

Description

This replaces Wasp CLI custom internal parsers with optparse-applicative for consistency. This also adds automatic bash, zsh and fish shell completions.

fixes #773

Command parser coverage progress

Command Implementation Unit Test (Parser) Note
new [name] [-t|--template]
version
waspls [--log LOG_FILE] [--stdio]
vscode passes --stdio to the language server. waspls always uses --stdio, so this switch is ignored. it's included for vscode compatibility
completion [generate (bash|zsh|fish)]
breaking change: use completion generate shell format instead of completion:generate:shell for consistency with the rest of CLI commands
uninstall
start [db]
we merge both start with and without db options together while maintaining same previous behavior
db (start|reset|seed [name]|studio|migrate-dev [--name migration-name] [--create-only])
compile
internally called by wasp start - see discord message
clean
build
deploy [args]
All the arguments will be supplied by external tool (handled by deploy tool of choice, by default it's currently with fly)
telemetry
deps
dockerfile
info
test (client) [args]

Misc

  1. Double check and fix help prompt messages.
  2. Reorganize files (specific command parser code goes into <COMMAND_NAME>/Parser.hs). Do we want to include a simple, single command parser like version and info though?
  3. Restore original command colors if possible.
  4. Rename variables and functions to follow the existing coding style. (not sure about pure vs return)
  5. Make unit tests description more meaningful, such as: db start -t shouldn't pass.

Select what type of change this PR introduces:

  1. Just code/docs improvement (no functional change).
  2. Bug fix (non-breaking change which fixes an issue).
  3. New feature (non-breaking change which adds functionality).
  4. Breaking change (fix or feature that would cause existing functionality to not work as expected).

Update Waspc ChangeLog and version if needed

If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following:

  1. I updated ChangeLog.md with description of the change this PR introduces.
  2. I bumped waspc version in waspc.cabal to reflect changes I introduced, with regards to the version of the latest wasp release, if the bump was needed.

modotte added 29 commits May 15, 2023 19:48
- We still doesn't handle the command runner yet in this commit.
The commands are supplied externally somewhere! So we don't need to
treat running `wasp deploy` alone as error.
Need to implement custom reader for `completion:generate:shell`
argument format.
@Martinsos Martinsos requested a review from infomiho May 22, 2023 12:47
- Replace external commands with locally defined commands.
  We only import the command parsers from outside the file.

- Rename parser to use Parser suffix for consistency.

- Rename mkNormalCommand to mkCommand and mkWrapperCommand to
  mkCommandWithInfo.

- Also rename mkCommand and mkCommandWithInfo parameters to adjust with
Parser suffix.

- Temporarily rename parserSuite to topLevelCommandsParser.
Copy link
Member

@Martinsos Martinsos left a comment

Choose a reason for hiding this comment

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

@modotte I reviewed your new changes!

There are some comments you haven't addressed yet, without counting the big one with refactoring that we are leaving for the last.
If you are not sure which ones are still open, go through Conversations and you should find them.

We are coming close to an end I think, so I would love us to wrap this up and merge it! If any of open comments seems like too much work / troublesome, do pls comments so and we can figure out what to do with it.

@ghost
Copy link
Author

ghost commented Jun 2, 2023

@Martinsos Hi again. Thx for all the reviews. I've addressed most of the comments including the (COMMAND | COMMAND) help output mentioned yesterday. I've also restored EXAMPLES in front of the main help page (I forgot to put it back, and it just makes sense to have it back as a guide for new users anyways).

The last big one (architecture) and the color of the commands (which is probably better in the long-term if solved directly in optparse-applicative) are the major ones that are still left to be solved.

@Martinsos
Copy link
Member

Martinsos commented Jun 2, 2023

Awesome @modotte and thanks for this nice summary, it helps a lot with reviewing!

  • Big refactoring -> I commented on it -> up to you, you can resolve it if you wish! I will let you make the final call.
  • Colors -> well, this one is tricky. I am a bit hesitant to merge this PR until we actually have that solved, since we would regress in not having nice visuals. I agree it would be best to get support for it from optparse, that is a long term solution. If that doesn't work though, do you have any ideas for a hacky solution that could work? Can we intercept the string it returns for help and modify it / decorate it, even if with somewhat fragile logic?

Btw you can freely undraft this PR, it has not been a draft for a long time I think :D.

@ghost
Copy link
Author

ghost commented Jun 2, 2023

Alright! I'll take a look at what I can do in the meantime to get the command colors back again. 🙂

@ghost ghost changed the title Draft: Make all wasp cli commands to use optparse-applicative Make all wasp cli commands to use optparse-applicative Jun 2, 2023
@ghost ghost marked this pull request as ready for review June 2, 2023 18:19
@Martinsos
Copy link
Member

Alright! I'll take a look at what I can do in the meantime to get the command colors back again. slightly_smiling_face

I just took a quick look myself -> doesn't feel like much can be done? I continued discussion on the optparse repo, I think the only way through this is that we make a PR there to implement this.

@Martinsos
Copy link
Member

Since coloring is main thing stopping us from merging this PR, I took a bit of time to play with trying to extend optparse-applicative to support cusotmization of help, here is some very early draft: pcapriotti/optparse-applicative#482 .

@Martinsos
Copy link
Member

I wouldn't close this one as we are not giving up on it -> it will need updating, and we still need to get stuff done in optparse first, but we will do it at some point.

@ghost ghost closed this by deleting the head repository Mar 8, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Wasp's CLI bash completion list update dynamically based on the commands Wasp CLI implements
3 participants