Improve various CLI command error messages

[sellout]

Overview

.> lib.install blah.blah.blah

    Sorry, I couldn’t understand your request. Expected a project or branch, but I saw
    “blah.blah.blah”.
    
    Usage:
      The `lib.install` command installs a dependency into the `lib` namespace.
      
      `lib.install @unison/base/releases/latest`  installs the latest release of `@unison/base`
      `lib.install @unison/base/releases/3.0.0`   installs version 3.0.0 of `@unison/base`
      `lib.install @unison/base/topic`            installs the `topic` branch of `@unison/base`

All UCM error messages now start with “Sorry, I couldn’t understand your request.” and end with the command’s help text. A specific failure reason is included in between.

This hopefully makes things more informative for the user without overwhelming them with output. Previously, many failures simply reported the help text with no other context. This has confused me in the past when I wasn’t sure what was wrong with my input, and it looked like UCM just interpreted .> foo bar as .> help foo.

Interesting/controversial decisions

I feel like all of this text generation should be moved out of this module, producing a sum type instead of Pretty output. And things like the “wrong number of args” can be produced from the ArgumentTypes in the InputPattern. Possibly the whole parse function can be produced that way. But that is work for another time.

[aryairani]

I think this is a good idea and heading in the right direction.

I hadn't noticed how weird a lot of the existing messages were, but I think we should take the opportunity to make them even less weird. I'd like to iterate a little more, either together or separately, but something that would be a big help for that would be if we had a transcript that demonstrated all the different classes of error messages, so that we can edit them in context (instead of just kind of imagining the output from the Haskell source, like we have to in many cases.)

e.g.

Sorry, I couldn’t understand your request. Expected a project or branch, but I saw “blah.blah.blah”.

I see that this "Expected blah" text was already there, and shame on us for the incomplete sentence. I wanna stay away from the typical robot voice that most tools use, so maybe:

Sorry, I wasn't sure how to process the request. I was expecting <a whatever>, but I couldn't recognize "blah.blah.blah" as one.

It gets tricky though, as you know, to shoehorn the prose into a template without getting artificial-sounding output, but we can take a look.

[sellout]

I think this is a good idea and heading in the right direction.

I hadn't noticed how weird a lot of the existing messages were, but I think we should take the opportunity to make them even less weird. I'd like to iterate a little more, either together or separately, but something that would be a big help for that would be if we had a transcript that demonstrated all the different classes of error messages, so that we can edit them in context (instead of just kind of imagining the output from the Haskell source, like we have to in many cases.)

Yeah, this is one of the benefits of using structured types in the Left, and building the messages all in one place.

Honestly, though, some of those messages are my fault. I meant to come back to them in the original PR, but they were too far behind me by the time I wrapped it up.

Sorry, I couldn’t understand your request. Expected a project or branch, but I saw “blah.blah.blah”.

I see that this "Expected blah" text was already there, and shame on us for the incomplete sentence. I wanna stay away from the typical robot voice that most tools use, so maybe:

Whoops, I think I copy/pasted from an earlier build. I got closer to your suggestion before opening the PR: https://github.com/unisonweb/unison/pull/5054/files#diff-4927adadae99f150f6af96ec90f31693b28333ddd5bd3f6c83d701bee193e826R331 but still not quite there.

Sorry, I wasn't sure how to process the request. I was expecting <a whatever>, but I couldn't recognize "blah.blah.blah" as one.

Yeah, this is better.

[mitchellwrosen]

I'm all for improving the error messages here, but I had thought @aryairani created https://github.com/unisonweb/unison/issues/4978 partially in response to a new message I drafted that shows the help and an error message. So, we should decide one way or the other whether we always show the help on a parse error, or never.

[mitchellwrosen]

The Usage: followed by indented help is a nice touch though, maybe that sort of fixes the issue of the error getting buried, as in this old pull output I referred to above:

  The `pull` command merges a remote namespace into a local branch
  
  `pull @unison/base/main`                merges the branch `main` of the Unison Share hosted
                                          project `@unison/base` into the current branch
  `pull @unison/base/main my-base/topic`  merges the branch `main` of the Unison Share hosted
                                          project `@unison/base` into the branch `topic` of the
                                          local `my-base` project
  
  where `remote` is a project or project branch, such as:
    Project (defaults to the /main branch) `@unison/base`
    Project Branch                         `@unison/base/feature`
    Contributor Branch                     `@unison/base/@johnsmith/feature`
    Project Release                        `@unison/base/releases/1.0.0`
  
  
  You may only `pull` into a branch. Did you mean to run
  `lib.install @unison/base/releases/latest`?

[aryairani]

Yeah I don't want the issue to get buried, so I would only say "always show usage" if we also have very concise usage examples to provide. "Never show usage" also seems wrong though.

[mitchellwrosen]

I like "always show usage", especially if "usage" is different from the "help" output, which could then be arbitrarily verbose, since the user is explicitly asking for it.

[sellout]

Well, I think for now we could just suffix “Use help {{command}} to see more about how to use {{command}}.” instead of the full help message. Then later we can figure out something about including the help instead of the rendered size is < n lines or something.

[aryairani]

Should we turn on CI for PR commits as well as for merge commits? I vaguely remember there being some issue with that, though, apart from doubling the number of runners and runtime.

[sellout]

Should we turn on CI for PR commits as well as for merge commits? I vaguely remember there being some issue with that, though, apart from doubling the number of runners and runtime.

I think one of the benefits of a merge-based workflow is that the lines of development remain independent as much as possible. Merging trunk into the branch undoes a lot of that. But I also don’t want to be a fly in the ointment – I’m happy to merge into my branches more frequently, just a habit to build.

[sellout]

Just a ping for review here. I also have some other work in draft that will conflict with or depend on this, but it’d be better to get this one in first.

The last changes to this don’t include the help output with errors, but instead recommend running help.

[aryairani]

Just a ping for review here. I also have some other work in draft that will conflict with or depend on this, but it’d be better to get this one in first.

The last changes to this don’t include the help output with errors, but instead recommend running help.

Sorry yeah — it looks good, but maybe needs transcript outputs checked in again?

[aryairani]

I tried to update the transcripts to get this merged, but ran into a glitch when I did so:

The help command uses Left to print the help text back to the user instead of a returning a Right Input; resulting in output like:

which we don't want. In other words, Left doesn't always currently mean error; it just means "print a message to the user".

Complicating matters further, it looks like there are a bunch of commands that already try to print their own help in that message, resulting in doubly-printed help or weird messages:

Unfortunately we don't have a transcript that shows all this, or it would have been more obvious earlier. 😅

I tried to do a quick fixup, but also ran into a circular dependency issue, trying to use some of the message-creation helpers of InputPatterns.hs (e.g. makeExample*) to build the generic messages in CommandLine.hs, which InputPatterns.hs depends on for its message-creation helpers. I'm moving them out now.

[sellout]

Ugh, one of these issues is mine (although I swear I remember making this exact fix before my last commit 😬) and another is due to the merge, I think.

help using Left for its successful output is an actual issue, that I overlooked, though. It seems like someone already dealt with this problem, and added Input.CreateMessageI, so Right . CreateMessageI should just be used instead of Left in cases like help and help-topics.

I can have a fix up for all these cases in a minute. And sorry for having you dig into this before I got back to it.

[aryairani]

That's okay, digging in yesterday made me think the problem is actually bigger than I realized, due to general inconsistency on how all the commands produce their output. Meaning the idea here is still good, but the solution is harder than we realized. I was thinking we could talk through it on Monday? I can have you take a look at my attempted fixes and let me know what you think. Though, CreateMessageI seems like a good idea.

[sellout]

Ok, this includes your changes, plus some others. No more duplicated help output, no more ruining output by wrapping it, etc.

Here’s an updated transcript output with the examples you included above.

.> help merge

  merge
  `merge /branch` merges `branch` into the current branch

.> help merge.commit

  merge.commit (or commit.merge)
  `merge.commit` merges a temporary branch created by the `merge` command back into its parent
  branch, and removes the temporary branch.
  
  For example, if you've done `merge topic` from main, then `merge.commit` is equivalent to doing
  
    * switch /main
    * merge /merge-topic-into-main
    * delete.branch /merge-topic-into-main

.> names a b

  ⚠️
  
  Sorry, I wasn’t sure how to process your request.
  
    I expected exactly one argument, but received 2.
  
  You can run `help names` for more information on using `names`.

.> todo haha

  ⚠️
  
  Sorry, I wasn’t sure how to process your request.
  
    I expected no arguments, but received 1.
  
  You can run `help todo` for more information on using `todo`.

.> merge.commit main

  ⚠️
  
  Sorry, I wasn’t sure how to process your request.
  
    I expected no arguments, but received 1.
  
  You can run `help merge.commit` for more information on using `merge.commit`.

[sellout]

Here’s another that shows something other than “wrong number of args”.

.> pull foo .bar

  ⚠️
  
  Sorry, I wasn’t sure how to process your request.
  
    I think you want to merge foo/ into the .bar namespace, but the `pull` command only supports
    merging into the top level of a local project branch.
  
  You can run `help pull` for more information on using `pull`.