Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make implementing Helper easier #207

Open
HarrisonMc555 opened this issue Mar 23, 2019 · 11 comments
Open

Make implementing Helper easier #207

HarrisonMc555 opened this issue Mar 23, 2019 · 11 comments

Comments

@HarrisonMc555
Copy link

I am just starting to look at rustyline, and it looks good so far. However, I decided that I might want to try to implement a simple Hinter.

As far as I understand it, every Editor has an associated Helper type. This can be (), which has default (empty) implementations of the required Completer, Hinter, and Highlighter traits. However, if you want to implement your own Helper, that type needs to implement all three of these traits. Doing so requires a lot of boilerplate code if you want "no-op" implementations of certain traits.

Is there any way we could ease this and make it so that if you only wanted to implement one of these, you could do so? If I understand correctly, I almost think you could move the current implementations for () into the trait definitions to give them default implementations. I'm not 100% sure about associated types (i.e. Completer), but the rest should be fine.

@gwenn
Copy link
Collaborator

gwenn commented Mar 23, 2019

I don't know:
#197 (comment)
Maybe introduce some features ?

@HarrisonMc555
Copy link
Author

Oh that makes it a lot easier. The only one that's not quite as easy is the Completer. I think we would still have to declare the associated type (String is what I did since it has a pre-defined implementation), but I believe the required methods for Completer could have default no-op implementations.

@oppiliappan
Copy link

A Helper shouldn't have to have all three trait implementations.
Maybe we can have something like set_hinter, set_highlighter and set_completer?

Right now, its a little roundabout to implement just one of the three, especially if I want to skip the Completer

@gwenn
Copy link
Collaborator

gwenn commented Jun 18, 2019

For one syntax, Hinter, Highlighter and Completer, Validator should/would/will be correlated.
Because they should/would/will share the same Tokenizer/Parser.
Currently, each one parses the same input...
See #197 (comment)

I guess that this cannot be done if we introduce set_hinter, ...

@gwenn
Copy link
Collaborator

gwenn commented Aug 15, 2019

See #267

@gwenn gwenn removed the help wanted label Aug 15, 2019
@thomcc
Copy link
Contributor

thomcc commented Jan 13, 2020

Maybe I'm missing something, but why are these all separate traits instead of just one trait with defaulted functions? It seems like the separation just means that each additional feature is a semver-breaking change, and it adds boilerplate to implementers.

@gwenn
Copy link
Collaborator

gwenn commented Jan 13, 2020

@thomcc Because you may want to reuse the FilenameCompleter or the HistoryHinter but implement your own highlighter. Or someone may implement a reusable highlighter based on syntect/tree-sitter by specifying only the language extension...

@thomcc
Copy link
Contributor

thomcc commented Feb 12, 2020

Yeah, so I don't really see why those are incompatible. In fact, the way it is now makes it more annoying to reuse, since if even when you do reuse those things, you need to forward stuff along.

Here's one option:

pub trait Helper {
    fn hinter(&self) -> Option<&dyn Hinter> { None }
    fn highlighter(&self) -> Option<&dyn Highlighter> { None }
    fn validator(&self) -> Option<&dyn Validator> { None }
    fn completer(&self) -> Option<&dyn Completer> { None }
}

Then to reuse things, you would just return Some(&self.hinter) rather than having to implement several methods that forward the arguments to self.hinter.

(The wrinkle here is that this works for all of these except Completer, but I think that can be made to work too).

There are other possible designs, but the nice associated type one that comes to mind doesn't actually work, due to associated type defaults not being a thing yet.

The benefit here is that you get an easier to implement Helper type, and you don't have to bump major versions every time you add a new feature which would require something new of Helper.

@gwenn
Copy link
Collaborator

gwenn commented Feb 12, 2020

@thomcc
Yes, your solution should work.
We will need something like that:

pub trait Helper {
    type Document;
    
    fn lexer(&self) -> Option<&dyn Lexer<Self::Document>> { None }
    fn hinter(&self) -> Option<&dyn Hinter<Self::Document>> { None }
    fn highlighter(&self) -> Option<&dyn Highlighter<Self::Document>> { None }
    fn validator(&self) -> Option<&dyn Validator<Self::Document>> { None }
    fn completer(&self) -> Option<&dyn Completer<Self::Document>> { None }
}

pub trait Lexer {
    type Document;

    fn lex(&self) -> Self::Document;    
}

pub trait Completer {
    type Candidate: Candidate;
    type Document;

    fn complete(&self, doc: Self::Document, ...) -> Result<(usize, Vec<Self::Candidate>)> ...
}
...

@thomcc
Copy link
Contributor

thomcc commented Feb 12, 2020

Hmm, I see... I saw you refer to this in the BUGS doc, and this refers to a similar architecture to https://python-prompt-toolkit.readthedocs.io/en/master/pages/advanced_topics/rendering_flow.html#the-rendering-flow ?

  1. I think the optimization that allows for incremental parsing is probably pointless -- These input buffers are almost always small. THe bottlenecks has definitely been writing stuff to the terminal from what I've seen. (And semantic analysis -- but nothing rustyline does will speed that up)
    • That said, there are definitely parts of their stuff that seem like they'd help here -- buffering, change tracking, etc.
  2. I really like that they manage styling -- IMO rustyline should do that too, rather than have you write escapes and hope for the best.
  3. The other thing I like is that each step (each Processor) seems to be able to update a lot about the output things -- When working on evcxr's repl, I found it frustrating that I couldn't modify the styling info from inside the validator.

I'll think about it. I suspect that you probably don't need to make document fully generic -- I can always build it internally if I need it everywhere. I'll think about it though.

@gwenn
Copy link
Collaborator

gwenn commented Feb 13, 2020

@thomcc Only the highlighter should modify the styling because the terminal or the user doesn't want ANSI code...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants