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

mutable Helper in functions #809

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zao111222333
Copy link

@zao111222333 zao111222333 commented Oct 5, 2024

See gwenn#4
And I add a new example/bracket_parser.rs to demo Parser trait.
bracket_parser

btw, rightnow I have no idea how to represent change: InputEdit.

@gwenn
Copy link
Collaborator

gwenn commented Oct 5, 2024

Thank you for all your proposals.
But please only one feature at a time: either mut Helper or Parser but not both in the same PR.

@@ -482,7 +492,7 @@ fn apply_backspace_direct(input: &str) -> String {
fn readline_direct(
mut reader: impl BufRead,
mut writer: impl Write,
validator: &Option<impl Validator>,
mut validator: Option<&mut impl Validator>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why you need the mut validator

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The match block requires the input validator is mutable. And when we call it, use readline_direct(io::stdin().lock(), io::stderr(), self.helper.as_mut()), it is almost same as before

rustyline/src/lib.rs

Lines 519 to 523 in 77e7d3d

match validator {
None => return Ok(input),
Some(ref mut v) => {
let mut ctx = input.as_str();
let mut ctx = validate::ValidationContext::new(&mut ctx);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

src/edit.rs Outdated
self.line.as_str()
}
}
// We only use `self.line` to create `impl Invoke`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See

// TODO
//fn invoke(&mut self, cmd: Cmd) -> Result<?> {
// self.i.invoke(cmd)
//}

@@ -102,32 +102,42 @@ pub fn highlighter_macro_derive(input: TokenStream) -> TokenStream {
quote! {
#[automatically_derived]
impl #impl_generics ::rustyline::highlight::Highlighter for #name #ty_generics #where_clause {
fn highlight<'l>(&self, line: &'l str, pos: usize) -> ::std::borrow::Cow<'l, str> {
::rustyline::highlight::Highlighter::highlight(&self.#field_name_or_index, line, pos)
#[cfg(any(not(feature = "split-highlight"), feature = "ansi-str"))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No split-highlight in this PR please

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for that, I directly copy rustyline-derive/src/lib.rs for the old version. I only copy this single file and I will remove those :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worry !

Copy link
Collaborator

@gwenn gwenn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove all Parser / split-highlight related code

@gwenn
Copy link
Collaborator

gwenn commented Oct 5, 2024

See

enum Change {
for a possible InputEdit impl (except for Begin / End)

@zao111222333
Copy link
Author

But please only one feature at a time: either mut Helper or Parser but not both in the same PR.

Understand, and since the Parser::parse requires &mut self, I think I can firstly fix all issue (like create ValidationContext from State) for mut Helper. Merge it firstly.
And then create another PR for Parser (as well as InputEdit)

@zao111222333
Copy link
Author

Here is the update under your suggestions.
I think the Invoke should impl for LineBuffer and String, rather than State and &str.
The &str could be replaced to String since itself will be changed in invoke.
And State could be replaced to LineBuffer since Invoke seems only require LineBuffer.
How do you think that?

I haven't fixed fn invoke(&mut self, cmd: Cmd) in this commit, I will try to implement this feature in future.

@zao111222333 zao111222333 changed the title Parser trait & mutable Helper in functions mutable Helper in functions Oct 5, 2024
@gwenn
Copy link
Collaborator

gwenn commented Oct 5, 2024

You need the State to invoke a Cmd on a LineBuffer for Undo / Kill ring.
Like Changeset here:

rustyline/src/completion.rs

Lines 102 to 105 in ceafb7f

fn update(&self, line: &mut LineBuffer, start: usize, elected: &str, cl: &mut Changeset) {
let end = line.pos();
line.replace(start..end, elected, cl);
}

@zao111222333
Copy link
Author

zao111222333 commented Oct 5, 2024

Or at least use a part of State to create a ValidationContext? Since here we already make a mutable reference from State.helper, then it's difficult to have another mutable refence from entire State (will be ValidationContext::new(self) in code).

rustyline/src/edit.rs

Lines 250 to 253 in 8fc0037

if let Some(ref mut validator) = self.helper {
self.changes.begin();
let result =
(*validator).validate(&mut ValidationContext::new(&mut self.line))?;

Now I am not sure which State attributs are needed for fn invoke.

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