Refactor the Nickel CLI

[vkleen]

This is a refactor of the Nickel CLI structure with the goal of easing maintenance and later on streamlining the user interface. Notable changes are:

• Create an explicit eval subcommand, while keeping the eval the default when no subcommand is provided
• Add an option to generate shell completion files: nickel gen-completions bash, for instance.
• Restructure the CLI code into one Rust module per subcommand.
• Create a structured CLI error type

Depends on #1486

[yannham]

I haven't looked at it yet, but this PR looks like it mixes many different things - code refactoring, changing the interface, adding new features wrt merging - would it be feasible to break it down into as much independent PR as possible, if reasonable (that is, if that doesn't incur too much work)? In particular, I would be happy to merge refactoring ASAP, but redesigning the UX probably mandates more research and discussion.

[vkleen]

That makes sense, I'll try to make this PR only a refactoring without changing the interface.

[dpulls]

🎉 All dependencies have been resolved !

[vkleen]

The current version should now keep the previous user-facing structure with two exceptions. First, there is hidden support for an eval subcommand which is still the default when no subcommand is specified. Second, there is a gen-completions subcommand using clap_complete to generate shell completion files.

[yannham]

After installing this version, I don't get any completion in ZSH. Is there any step that I'm missing to get it to work?

[yannham]

I wonder if the naming is correct, as this is clearly an extension trait (cf https://rust-lang.github.io/rfcs/0445-extension-trait-conventions.html). Given the linked RFC, this might probably be called ResultErrorExt or maybe CliResultExt, even if CliResult is defined within this crate...or, ResultErrorExt? The latter is probably the closest to the RFC, but it's really obscure.

Also, I feel like the T should be a parameter of with_program and not of WithProgram, but it's purely aesthetic, as I believe the two version are isomorphic.

Another solution is to make with_program a free-standing function, which is perhaps less surprising but you lose the ability to chain with .. Or to simply inline with_program, which doesn't sound too bad either.

[vkleen]

I'm not sure WithProgram really is an extension trait here. The point of e.g. IteratorExt is that anything that implements Iterator also implements IteratorExt and the split is purely for object safety reasons.

Making T just a parameter of the function doesn't work, I think, because I'm implementing WithProgram for Result<T, _> and the type parameter can't actually change for each invocation of with_program. That is to say, this would really be trait for things of kind Type -> Type. But that can't actually be expressed in Rust traits at the moment, as far as I know.

My idea here was to get as close to just a ? at the use site as possible. Just implementing an instance of From isn't good enough here because I need to provide the Program that is to be used for reporting the error. Inlining with_program ends up exposing the structure of the error type at each use site, which feels a bit wrong to me.

[vkleen]

That being said, with_program might be too generic a name for this function. Perhaps report_with_program or just report_with could be more readable at the use sites.

[yannham]

Making T just a parameter of the function doesn't work, I think, because I'm implementing WithProgram for Result<T, _> and the type parameter can't actually change for each invocation of with_program. That is to say, this would really be trait for things of kind Type -> Type. But that can't actually be expressed in Rust traits at the moment, as far as I know.

Ah, good point.

My idea here was to get as close to just a ? at the use site as possible. Just implementing an instance of From isn't good enough here because I need to provide the Program that is to be used for reporting the error. Inlining with_program ends up exposing the structure of the error type at each use site, which feels a bit wrong to me.

I see your point, but I would argue that it's an extension trait currently because your return type is CliResult<T> and it seems to me that the only obvious implementer is CliResult<T>. I don't really see what other type would or could implement it, excepted maybe something that converts to CliResult<T>. Put differently, had we defined CliResult as a newtype wrapper, I believe with_program would naturally be a method of CliResult rather than having its own trait.

As per the RFC:

Of course, adding methods via a new trait happens all the time. What makes it an extension trait is that the trait is not designed for generic use, but only as way of adding methods to a specific type or family of types.

This is of course a somewhat subjective distinction. Whenever designing an extension trait, one should consider whether the trait could be used in some more generic way. If so, the trait should be named and exported as if it were just a "normal" trait. But traits offering groups of methods that really only make sense in the context of some particular type(s) are true extension traits.

But, as mentioned in this extract, this is a subjective distinction. All in all this is not very important and I'll let you decide what you think is best. I just wanted to bring the attention to existing conventions.

[vkleen]

It's not CliResult<T> that implements the trait but Result<T, nickel_lang_core::error::Error>. Note the different Error type. In a way, it's really a specialized Into that needs to take an extra parameter. But I do see the point about it maybe being an extension trait per the RFC now. And I really don't see how this particular trait could be generically useful 😅

So, I agree with renaming it to something like ResultErrorExt. Naming things is truly one of the hard problems 😆

[yannham]

Sorry, this maybe wasn't ready for review yet 😅

[vkleen]

The more reviewing the better 😆 To get zsh completions, you'd need to source the output of nickel gen-completions zsh. There seems to be unstable dynamic completion support for clap, but that would still need to be registered with zsh and it would mostly be useful for tab-completing the new freeform arguments to export (and maybe eval etc.) that we're planning.

[vkleen]

About the completions, I actually can't get them to work ephemerally 😞 But nickel gen-completions zsh > ~/.config/zsh/vendor-completions/_nickel followed by compinit does give me completion in zsh.

[yannham]

Ah, it did work for me with source as you proposed, using nickel gen-completions zsh > completions.sh && source completions.sh && rm -f completions.sh or something like that.

[vkleen]

Oooh, thanks 👍 Even nicer for zsh: source <(nickel gen-completions zsh). That let's zsh handle the temporary file.