Restructured how commands like `list`, `help`, `quit` are parsed to implement `save`

[rben01]

Closes #187

To implement the save command, I implemented a simple command parser. This handles the following commands:

• help
• info
• clear
• quit / exit
• list / ls
• save (new)

These were formerly implemented as a bespoke command matcher, matching on e.g., "list functions" | "ls functions". Now they are fully parsed, which both enables more flexibility such as additional whitespace between arguments, and error reporting in the event that a command is misused. For instance, before, list foo would result in

>>> list foo
error: while type checking
  ┌─ <input:2>:1:1
  │
1 │ list foo
  │ ^^^^ unknown identifier
  │
  = Did you mean 'pint'?

Now it results in

>>> list foo
error: while parsing
  ┌─ <input:1>:1:6
  │
1 │ list foo
  │      ^^^ Invalid command: if provided, the argument to `list` or `ls` must be one of: functions, dimensions, variables, units

And

>>> help sin
error: while type checking
  ┌─ <input:4>:1:1
  │
1 │ help sin
  │ ^^^^ unknown identifier
  │
  = Did you mean 'hex'?

is now

>>> help sin
error: while parsing
  ┌─ <input:6>:1:6
  │
1 │ help sin
  │      ^^^ Invalid command: `help` takes 0 arguments; use `info <item>` for information about an item

Similar error reporting is implemented for the other commands.

But all of this was merely done to provide an easy way to add the save command — I didn't just want to continue down the path of ad-hoc string parsing. That said, currently the argument to save must have no whitespace characters in it, or else it will parse as multiple arguments. Nor does it process backslash-escape characters. I was debating whether to use Tokenizer to parse the argument the same way a String is parsed, but I figured I'd keep this PR small. That can be added later.

The save command now looks like

save hist.nbt

This will unconditionally overwrite the specified file if it exits. If writing fails, the error looks like

>>> save .
error: runtime error
 = Could not write to file: "."

Some improvements that might be desirable in the future:

• Filenames with spaces, presumably using ordinary string syntax (which would also handle backslash escape sequences)
  • Interpolation? Not sure how much utility this would have but it would be natural if using strings as an argument
• Confirmation when overwriting an existing file
• Omitting lines that errored. IIUC this would require some changes to how history is stored, since it only stores strings and not results
• Save per-session history, not entire history over all sessions ever

[sharkdp]

Thank you very much for your contribution — great changeset!

I implemented a simple command parser

Thanks, this is something I had planned but never got around to it :heart_eyes:. One huge benefit of your approach is that command parsing moves from the application (numbat-cli) to the library (numbat). This opens the possibility to offer command parsing / handling to other "frontends". For example, I think we could simplify this JS code as well:

https://github.com/sharkdp/numbat/blob/8c4d563233d4e1d20edf2cc55ec1733268761feb/numbat-wasm/www/index.js#L41-L76

In order to reduce code duplication even more (among frontends), we could potentially even move part of the command handling to the backend (library) as well. The whole "list xyz" => call ctx.print_xyz() logic, for example.

One complication is the following: not all frontends support the same set of commands. save would not be supported by the Web frontend, for example. And some commands definitely need frontend-specific code. Like a future copy command (#394).

I'm not suggesting that this should happen in this PR, by the way. Only if you are interested.

Now they are fully parsed, which both enables more flexibility such as additional whitespace between arguments, and error reporting in the event that a command is misused.

Very nice

That said, currently the argument to save must have no whitespace characters in it, or else it will parse as multiple arguments. Nor does it process backslash-escape characters. I was debating whether to use Tokenizer to parse the argument the same way a String is parsed, but I figured I'd keep this PR small. That can be added later.

I agree.

This will unconditionally overwrite the specified file if it exits.

I think that's completely fine. And maybe even a better UX than asking (interactively) whether the file should be overwritten or not.

[rben01]

One complication is the following: not all frontends support the same set of commands. save would not be supported by the Web frontend, for example. And some commands definitely need frontend-specific code. Like a future copy command (https://github.com/sharkdp/numbat/issues/394).

I will think about how to implement frontend-specific code for a separate PR. Where is the frontend we're using stored at runtime?

[rben01]

Also, regarding this:

This will unconditionally overwrite the specified file if it exits.

I think that's completely fine. And maybe even a better UX than asking (interactively) whether the file should be overwritten or not.

I was also considering having save and save!, the former asking for confirmation if overwriting and the latter overwriting without confirmation. If desirable, then I should probably change save to save! in this PR and the come back and implement the save command in the future. This might be a desirable change anyway just to indicate that we're going to overwrite (exclamation points indicate convey that something scary is happening) — users may exercise a tad more caution if it's made clear that they aren't getting a second chance.

[rben01]

In a future PR I'd also like to implement better discoverability of commands. Either help commands or list commands would be a good addition to actually list them all similar to what you'd get from cmd --help on the command line, and shouldn't be hard to do if we're willing to hard-code them, which seems perfectly reasonable. I would lean toward help commands just because list seems to be for runtime-specific values, whereas commands live in a totally separate namespace. I also think it'd make sense to list commands, or at least how to get a list of them, in help’s output. But again, future PR.

[sharkdp]

I was also considering having save and save!, the former asking for confirmation if overwriting and the latter overwriting without confirmation. If desirable, then I should probably change save to save! in this PR and the come back and implement the save command in the future. This might be a desirable change anyway just to indicate that we're going to overwrite (exclamation points indicate convey that something scary is happening) — users may exercise a tad more caution if it's made clear that they aren't getting a second chance.

I think I would vote for keeping things easy and just using save with a overwrite-default.

In a future PR I'd also like to implement better discoverability of commands. Either help commands or list commands would be a good addition to actually list them all similar to what you'd get from cmd --help on the command line, and shouldn't be hard to do if we're willing to hard-code them, which seems perfectly reasonable. I would lean toward help commands just because list seems to be for runtime-specific values, whereas commands live in a totally separate namespace. I also think it'd make sense to list commands, or at least how to get a list of them, in help’s output. But again, future PR.

Sounds good, but note that the commands depend on the frontend. Speaking of, we should also update the documentation:

https://numbat.dev/doc/cli-usage.html#commands https://numbat.dev/doc/web-usage.html#commands

see the corresponding markdown files

[sharkdp]

Save per-session history, not entire history over all sessions ever

Oh, I just found out about this after trying it. I think this is something we should implement before we merge this. Saving the entire history ever (1000 lines for me) is not very helpful.

Omitting lines that errored. IIUC this would require some changes to how history is stored, since it only stores strings and not results

I don't think we need to do that for the global history. But for the local one (for the save command), that would be useful. We do something similar in the web frontend:

https://github.com/sharkdp/numbat/blob/b175c315aee8eede9c481ac8e1b444b79d24867b/numbat-wasm/www/index.js#L72-L75

[rben01]

Oh, I just found out about this after trying it. I think this is something we should implement before we merge this. Saving the entire history ever (1000 lines for me) is not very helpful.

Shouldn't be hard to implement a per-session history. Two questions:

1. Should per-session history be a separate PR (to land before this one) or part of this one? This PR is already quite large, I'm thinking separate.
2. Should the implementation go in numbat or numbat-cli?

[sharkdp]

Sorry if that wasn't clear. I think ideally, the history as we have it right now on master should stay. This is helpful for Ctrl-R searching for commands from previous sessions.

But the save command should only store the commands of the current session. In order to turn a interactive session into a Numbat script that can be fine tuned in a text editor.

[sharkdp]

Thank you very much! Great work.