Skip to content

make match_any also return index of matched needle #22

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

Closed
wants to merge 2 commits into from

Conversation

philippkeller
Copy link
Collaborator

Public api stays the same except in exp_any which is additionally returning an index.

I'm not 100% happy with the change as I needed to add an additional function read_until_tuple_pos which serves as a common code base between the any-case and the other cases. This returns also the index which is 0 for the non-any cases.

But I couldn't come up with anything better. This is a breaking change so I bumped up the version.

This solves #14.

What do you think, @pbartyik, would this solve your use case?
Maybe @zhiburt, as you're most probably more into Rust than me at the moment, if you find anything which could be improved, then I'd be glad for input

…the same except in exp_any which is additionally returning an index
@philippkeller philippkeller mentioned this pull request May 24, 2020
@zhiburt
Copy link
Contributor

zhiburt commented May 24, 2020

Hi @philippkeller,
I suppose the #14 issue is actually a reasonable one.

It definitely caused my interest and a spend a little time on the draft of a possible design.
And first of all I really don't know the cost of this. And now I even think might I was involved not in the issue I should have (edited: definitely :( ) as we have this 3 parameters :) but that what I've got so far.

We could split the logic of ReadUntil to different types which united by Needle trait.

pub struct Match<T> {
    begin: usize,
    end: usize,
    interest: T,
}

pub trait Needle {
    type Interest;

    fn find(&self, buffer: &str, eof: bool) -> Option<Match<Self::Interest>>;
}

In this case each implementation of Needle would decide what parameters it should return.

read_until could look like that

    pub fn read_until<N: Needle + ?Sized>(&mut self, needle: &N)
       -> Result<(String, String, N::Interest)>

with such changes

            if let Some(m) = needle.find(&self.buffer, self.eof) {
                let first = ...;
                let second = ...;
                return Ok((first, second, m.interest));
            }

All that could safe the old interface or use a generic approach like that.

    fn test() {
        || -> Result<()> {
            let mut p = spawn("cat", Some(1000)).expect("cannot run cat");
            p.send_line("hello world!")?;
            p.exp("hello world!")?;
            p.send_line("hello heaven!")?;
            let arr = vec![reader::ReadUntil::EOF, ReadUntil::NBytes(10)];
            let (_, _, position) = p.exp(&arr[..])?;
            assert_eq!(position, 1);
            Ok(())
        }()
        .unwrap_or_else(|e| panic!("test_expect_string failed: {}", e));
    }

The 100% downside is the third parameter in most cases is () which is a pity

In any way the draft is passed all tests.

Copy link
Contributor

@zhiburt zhiburt left a comment

Choose a reason for hiding this comment

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

Pretty well done 💯

As I see find and now find_any functions are public, does it necessary to be pub?

Comment on lines +236 to +237
let first = self.buffer.drain(..tuple_pos.0).collect();
let second = self.buffer.drain(..tuple_pos.1 - tuple_pos.0).collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Might it would be better to move it in read_until_tuple_pos to remove a duplication in read_until_any.
In this case read_until_tuple_pos would return Result<(String, String, usize)>. And in read_until_any would be no need? Since read_until_tuple_pos would return exactly what it does now.
*after all the function could be renamed?

@@ -93,6 +93,15 @@ pub fn find(needle: &ReadUntil, buffer: &str, eof: bool) -> Option<(usize, usize
}
}

pub fn find_any(needle: &Vec<ReadUntil>, buffer: &str, eof: bool) -> Option<(usize, usize, usize)> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn find_any(needle: &Vec<ReadUntil>, buffer: &str, eof: bool) -> Option<(usize, usize, usize)> {
pub fn find_any(needle: &[ReadUntil], buffer: &str, eof: bool) -> Option<(usize, usize, usize)> {

Comment on lines +260 to +261
}
/// Read until needle is found and return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}
/// Read until needle is found and return
}
/// Read until needle is found and return

@philippkeller
Copy link
Collaborator Author

closing this in favor of #24 which solves this much nicer

@philippkeller philippkeller deleted the exp_any branch May 31, 2020 12:25
epage added a commit to epage/rexpect that referenced this pull request Nov 5, 2024
…mat-args

Have clippy warn about uninlined format arguments
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