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 exp_any return the earliest result #64

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

souze
Copy link
Collaborator

@souze souze commented Sep 12, 2022

Previously, exp_any would return the match with the lowest index in the list
So this
exp_any(&vec![ReadUntil::String("hello"), ReadUntil::String("hell")]

With the input from the process as "hello", will sometimes return "hell", and sometimes "hello", depending on how much the readbuffer happened to have collected.

@souze
Copy link
Collaborator Author

souze commented Sep 12, 2022

Is the only way to edit this commit-message to close the PR and open a new one?

@matthiasbeyer
Copy link
Member

No, you can git commit --amend locally and then force-push to this PR.

src/reader.rs Show resolved Hide resolved
@souze souze force-pushed the origin/any-returns-earliest-result branch from ce74614 to 834a9aa Compare September 12, 2022 08:50
Previously,
exp_any would return the match with the lowest index in the list.
So this:
exp_any(&vec![ReadUntil::String("hello"), ReadUntil::String("hell")]

With the input from the process as "hello",
will sometimes return "hell", and sometimes "hello".
Depending on how much the readbuffer happened to have collected.

With this change, the earliest match in the input will be matched.
That way, any difference in how much the read buffer has currently
consumed does not change the output of the exp_any function.

Signed-off-by: Erik Karlsson <[email protected]>
@souze souze force-pushed the origin/any-returns-earliest-result branch from 834a9aa to 642e718 Compare September 12, 2022 08:59
Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

👍 for also providing tests!

@matthiasbeyer
Copy link
Member

I guess we can

bors merge

@bors
Copy link
Contributor

bors bot commented Sep 12, 2022

Build succeeded:

@bors bors bot merged commit a277de4 into rust-cli:master Sep 12, 2022
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