Provide Distinct Builder for Reedline

[cactusdualcore]

In engine.rs there are a few methods whose docstrings start with the words "a builder to". The Builder pattern is
almost certainly a great fit for initializing a Reedline, but it is improperly implemented here.
I've checked online and all sources (including this one) noted or implied that a Builder is a separate type specially for complex initializations.

I see three problems with the current structure:

1. I found checking for multiple fully distinct ways to reach an identical result to be a good rule
   of thumb for whether something is well designed. I can think of at least two ways to branch the
   hinting strategy on some condition. The first is two write a hinter that looks at some state and
   then decides what to do from there. The second is to change the current hinter every time some
   state changes. Just considering my ability to reason about to code, I don't think the latter is
   ever a good idea.
2. When Reedline::create is called, it initializes with default values. This is great for quickly
   starting, but it somewhat conflicts with the idea of customizing an instance through a Builder.
   If nothing else, this might make some very unnecessary allocations for the defaults.
3. When building an instance initially, that instance is "under construction". This is conceptually
   different from a finished instance, so it probably shouldn't be the same type. This theoretically
   might even lead to issues when the instance is used before it is finalized.

Having a distinct Builder type makes sure instances of Reedline are finished for use and disallows
mutation during use.

This only marks the builder methods on Reedline as deprecated, so it doesn't break the API

[cactusdualcore]

I forgot to add documentation, which I think is important, so I won't mark this PR ready without it. I might add a (declarative) macro for the Builder as well, will see.

[fdncred]

Hey! Thanks for this! @stormasm and I can try to look through it, maybe others.

We're generally against macros unless they're undeniably intuitive and necessary, with the nushell team being the judge, of course.

I'm excited to dig through and see what you've come up with as an easier and more intuitive API.

[fdncred]

After skimming through the code quickly, this question comes to mind, if you wouldn't mind explaining a bit more. What makes these changes better or more intuitive for API consumers? They just look different to me but maybe I'm missing something.

[stormasm]

@Cactus-Man

I took a look at your PR --- and have a general sense of what you are trying to accomplish.

Thanks for putting in the effort !

Just an FYI for future PRs of this magnitude in Reedline...

It probably makes sense to discuss stuff like this ahead of time on our Discord Reedline Channel...
This way we can have some prior discussions and get a sense of where we are going with it...

Also --- the Reedline issues are also a good place to present the ideas...

With all of that said I will review the PR over the next number of days.

We have a release coming up next week so it will probably be after that..
As we would not make a decision on something like this prior to the release...

[cactusdualcore]

What makes these changes better or more intuitive for API consumers?

I think the largest improvement is naming consistency for related methods. Names should help choose which method to use for a purpose and disable_hints and a few more didn't really clearly express what they did. My intuition (and that of a peer I asked) was that it would temporarily disable the Hinter without clearing it. It also implies an enable_hints, which just doesn't exist. Implementing a consistent naming doesn't require this large change though. Note that I renamed create to new because together with the Default trait those are the de-facto standard for initialization. This allows users to work with their expectations and intuition.

In the current code, I implemented the MVP. There are probably more methods that could be beneficial on a builder. They are not strictly necessary to use it correctly though. Things like without_validation or without_custom_edit_mode are convenient but almost all of the time you can just put the corresponding line inside an if statement. Adding those methods would take the API surface for construction to roughly thrice it's current size. Reedline already has an extensive interface so I disliked cluttering it with 30 or more new functions. Components with exactly one underlying concept are much easier to understand and such methods might be convenient.

The generic with_foo<T>(T) style functions of the Builder also provide abstraction over the internal representation of the components. If somebody decided to use generics or &dyn Foo instead of Box<dyn Foo> the interface in the current main branch would need to be changed accordingly. The Builder is agnostic to the representation and just converts accordingly. Also, I think not having to Box on the API consumer side is nice.

In their functionality, most of the builder methods are setters. They allow changing internal state after initialization. I am uncertain whether that's what you want but given that there is at least one explicit setter (set_history_session_id, which btw does functionally the same as with_history_session_id), I don't think you want them to be setters. This is, of course an assumption, so I could be wrong here. I'd be happy about some feedback on this. I can think of a few subtle behavioral bugs when e.g. an validator changes between cycles. And if branching behavior is undeniably necessary, a quick validator impl can do the same without the possibility of those subtleties.

And, as I pointed out before, construction and workable are fundamentally different states and a shared representation just feels wrong. There's an excellent video on a vaguely related topic. I don't think using an object before it's finalized can lead to UB like with C++ constructors, but it can cause behavioral bugs so the explicit state change (also represented in the type system) helps with clarity. It also helps with confidence: If the Builder is passed to a library function it can't call Reedline::read_line or anything like that, so no need to worry about that.

[cactusdualcore]

I made some notes on what I want to improve. When I look at them now there are things that probably would have been less structural changes (like the consistent naming that I quickly hinted at in my previous message). Maybe not the best idea to start with this.

[cactusdualcore]

We're generally against macros unless they're undeniably intuitive and necessary, with the nushell team being the judge, of course.

A lot of the builder code is repetitive and a macro might simplify the structure. So it's definitely not necessary, but greatly reduces the amount of (similiar) code that has to be maintained.

[fdncred]

Thanks for the explanation. I appreciate the thoughtfulness.

[cactusdualcore]

The new commits introduce a macro for generalizing similarly structured methods. I put it together quickly and roughly, so this probably isn't the nicest it could be. Looking forward to feedback on it.

The macro generates a with_* and without_* for setting and resetting a field and a method for introspection currently.

I haven't finished the documentation yet. I think writing good documentation is difficult, and even more so with the macro generalizing the code.

[sholderbach]

Thanks for trying to tackle the inconsistencies in the API and going through all the config options. You are correct that our current configuration interface doesn't really meet the builder definition and also doesn't really provide assurances that things are properly configured.

With how we are handling the Reedline object in nushell we can't fully go ahead with the deprecations you suggest.
During each REPL iteration we in effect "mutate" certain parts of the line editor to provide updated completers/menus/etc.
Completely rebuilding doesn't work as we keep some state in the Reedline instance that is relevant (e.g. when clearing the screen above with Ctrl-L the internal LineBuffer is untouched so you can continue typing)

This would in effect kind of favor a fn ....(&mut self, ...) -> &mut Self API on Reedline for all those things we change via configuration in Nushell or are necessary to feed in new information. (returning &mut would just be for the chaining aesthetic :P, taking just &mut is a little bit nicer for conditional operations as well)

I think there could still be a place for a ReedlineBuilder so you can more easily arrive at a sane configuration especially for the more casual reedline users.

[cactusdualcore]

That's why I deprecated rather than removing the methods outright ;)

I still think that having a distinct builder and setters is an improvement because it allows for more granularity.

I haven't looked much at nushell, but I'll do that.

Oh, and could you give me some feedback on the macro?

[cactusdualcore]

I removed the macros, saving a dependency (paste) and split the builder-like API of Reedline into a seperate file.
Personally, I prefer the macro version, but I wouldn't reintroduce the paste dep.

With how we are handling the Reedline object in nushell we can't fully go ahead with the deprecations you suggest.

I haven't looked at nushell yet, so I can't say much about how the current iteration works with it. It almost definitely breaks.

Besides that, I tried to make this a little nicer with the points suggested.

[cactusdualcore]

(I think I did some general "tidying up" in some files alongside the actual code changes. I hope that's fine)

[cactusdualcore]

and split the builder-like API of Reedline into a seperate file.

The motivation for this is to

1. reduce the lines of code in engine.rs, which is a large file with a large amount of logic already. No need to clutter it with boilerplate
2. simplify future decisions for this API. Due to the way Rust handles impl blocks, the new location shouldn't change anything. I changed some signatures to increase consistency between the two APIs, which is a breaking change though.

[cactusdualcore]

With how we are handling the Reedline object in nushell we can't fully go ahead with the deprecations you suggest.
During each REPL iteration we in effect "mutate" certain parts of the line editor to provide updated completers/menus/etc.
Completely rebuilding doesn't work as we keep some state in the Reedline instance that is relevant (e.g. when clearing the screen above with Ctrl-L the internal LineBuffer is untouched so you can continue typing)

This PR is most likely not the place to discuss this, but there's a fundamental discussion to be had about what a prompt is. I think the current implementation is semantically a bit weird, as it mashes two answers together.