Skip to content

Allow the caller to supply their own Writer implementation.#4

Open
db48x wants to merge 1 commit into
m-lima:masterfrom
db48x:writer-trait-for-improved-output-control
Open

Allow the caller to supply their own Writer implementation.#4
db48x wants to merge 1 commit into
m-lima:masterfrom
db48x:writer-trait-for-improved-output-control

Conversation

@db48x

@db48x db48x commented May 7, 2021

Copy link
Copy Markdown

This lets the caller print the REPL in any style they want.

This lets the caller print the REPL in any style they want.
@m-lima

m-lima commented May 8, 2021

Copy link
Copy Markdown
Owner

Hey @db48x!
Thanks for the PR! This can be very useful if you want to do your own rendering.

My main concern is requiring nightly in the master branch. Not sure if I'm comfortable with that idea (I have a feeling you knew this comment was coming 😄). I'd be OK with it if this feature gate would be broken soon (Rust 2021?), but that doesn't seem to be the case 😞 RFC link

Maybe, to avoid this, as a first step, if read_line() accepted a custom Writer, then the Builder wouldn't need to be changed for now. This would avoid the default fn and the nightly requirement.
I created a branch based on your work to show what I mean. I will not merge it, but will leave there just as reference (this is your authorship, after all 😉) reference branch link

After that, if the Builder should control which Writer to use (which seems preferable), then the Builder can be typed to carry the Writer the line reader should use (similar to how the other overrides are done).

Though there's a catch! The errors returned by the Writer should be an associated type. We can't ask it to return a crossterm::ErrorKind, since you can't be sure that the custom Writer will use crossterm. And even if they did, you cannot build your own crossterm::ErrorKind errors.
So that means that the return type of read_line() will be typed.. This can become tricky..

An easy way out would be to make the error dynamic, with Box or &dyn. But whoever wrote the custom Writer knows exactly which errors they are returning, and it just sucks to have a library gulp that in and spit out an obscured error back to the author.

Do you agree with my line of thought? I hope I'm not overly concerned here.

My second concern is how deeply connected to crossterm rucline is. For example, the event loop outside of the rendering already requires crossterm in raw mode. Allowing the Writer to be anything would mean giving up control over the terminal state or messing up the custom Writer.
Do you think that it would be the case that writing a custom Writer would be so cumbersome that it almost makes no sense to use rucline in the first place?

I'm interested on how you envision this! I guess that your port of reposurgeon already put this in action!
Was it smooth to write your Writer or did you have to follow the built-in writer closely? If you weren't using rucline, would it be easier?

I'm glad to move this PR forward!

@db48x

db48x commented May 8, 2021

Copy link
Copy Markdown
Author

Yes, using specialization was a last–minute thing; when I started writing the Writer trait, I didn’t realize that it would be necessary. I’m open to any other method which lets us avoid needing it.

Make the error type an associated type is a very good idea as well; at the moment I my code can only return the same crossterm::ErrorKind as the default Writer, but that won’t necessarily always be the case. It’s worth thinking about ahead of time. Honestly, I’ve barely thought about errors in my application. In all the code I’ve written so far I’ve simply done whatever was easiest, without thinking about it much.

My port of Reposurgeon is using this, but only on a side branch: https://gitlab.com/db48x/reposurgeon/-/commits/rust-port-with-rucline. The whole thing is very much a prototype.

Writing the Writer here was very easy; I just copied RuclineWriter to start, and changed the name of the struct. Then I added code for printing the error messages and usage. I tweaked the way suggestions are printed so that it looks like a menu floating over the error messages and usage behind. And I changed what clear_from does, so that it doesn’t erase everything that’s just been printed.

@m-lima m-lima force-pushed the master branch 3 times, most recently from b3f19fe to 7b94560 Compare June 10, 2022 22:24
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.

2 participants