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

Add option to skip ansi escape codes #103

Merged
merged 4 commits into from
Oct 27, 2023

Conversation

phaer
Copy link
Contributor

@phaer phaer commented Apr 23, 2023

This allows users to ignore ansi escape codes, such as terminal colors and would therefore close #25.

As you've noticed, i needed to add the new parameter skip_ansi_escape_codes in quite a few places. Maybe it would be a good idea to introduce an Options struct for timeout and skip_ansi_escape_codes, with default values? I could do that, but wanted to keep this PR focused.

I've considered using strip_ansi_escapes, mentioned in the issue. But that uses VTE which uses a state machine to interpret the escape codes.

But as we are not interested in interpreting them, just skipping them and ansi escape codes are just the byte 27 inserted in a string, followed by "[", zero or more numbers separated by ";" and ending with a letter;
We optionally just skip those in our non-blocking reader in order to efficently ignore them during matching without having to traverse the whole buffer a second time. A minimal test case is also added

@petreeftime
Copy link
Collaborator

Adding an optional Options or Config generated by a ConfigBuilder would be ideal, so that we don't have to change the API for every field.

I would also add a spawn_with_config function instead of modifing the API of the current spawn function.

@phaer
Copy link
Contributor Author

phaer commented Apr 23, 2023

Thanks for the quick review @petreeftime. I've added Options, which makes the diff much more readable.

The spawn* API stays the same, but gains spawn_with_options. Just saw that the test failed again, same failure as in my other PR - but I can't reproduce locally with cargo test yet 🤔

@matthiasbeyer
Copy link
Member

Yeah that CI failure is attempted to be fixed in #101 - it is some kind of sporadic failure which we did not find yet.

@phaer
Copy link
Contributor Author

phaer commented Apr 24, 2023

Ah, thanks for the explanation! So I'll just leave this PR-as is, as bors should re-run the checks before an eventual merge anyways, right?

LMK if there's something left to improve or fix :)

@matthiasbeyer
Copy link
Member

Yeah I think this is fine... we'll ping you if there's anything to fix up, I guess.

src/reader.rs Outdated Show resolved Hide resolved
@phaer phaer force-pushed the skip-ansi-escape-codes branch 4 times, most recently from 883b178 to b1b6044 Compare April 27, 2023 10:53
Copy link
Collaborator

@souze souze left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice work!

@phaer
Copy link
Contributor Author

phaer commented Jun 27, 2023

@matthiasbeyer Friendly ping :) could we merge this?

@matthiasbeyer
Copy link
Member

Yes we could, but we're getting the sporadic CI error again and for whatever reason I cannot re-run the CI.

Could you @phaer please rebase this PR to latest master, or, if it is based on latest master: git commit --amend --no-edit && git push <your github remote> <this branch> --force please? This way CI should re-run and I hope we do not get the error this time.

@matthiasbeyer
Copy link
Member

This CI failure is really annoying 😠

ansi escape codes are just the byte 27 inserted in a string,
followed by "[", zero or more numbers separated by ";" and ending
with a letter.
We optionally just skip those in our non-blocking reader in order
to efficently ignore them during matching.

Signed-off-by: phaer <[email protected]>
because handling of Err(_) and Ok(0) might be tricky otherwise,
so we just use a boolean flag

Signed-off-by: phaer <[email protected]>
@phaer
Copy link
Contributor Author

phaer commented Oct 27, 2023

@matthiasbeyer Just remembered this, rebased and we've got a passing CI now 🥳

@matthiasbeyer
Copy link
Member

bors merge=souze

@bors
Copy link
Contributor

bors bot commented Oct 27, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9eb61dd into rust-cli:master Oct 27, 2023
14 checks passed
@phaer phaer deleted the skip-ansi-escape-codes branch October 27, 2023 10:44
@hirrolot hirrolot mentioned this pull request Jan 12, 2024
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.

Strip ANSI escape codes
4 participants