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

Rework lessopen implementation to use execute crate instead of run_script #2805

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

Conversation

Anomalocaridid
Copy link
Contributor

I replaced the lessopen implementation to use a crate called execute instead of the currently-used run_script. This has the benefit of simplifying the code a little, but more importantly fixes a few weird issues I noticed.

The issues I noticed that these changes fix are:

  • batpipe not working right when using fish as a shell. Currently, bat shows the commands that batpipe prints to configure $LESSOPEN when you evaluate it in your startup config rather than the preprocessed file output.
  • bat not showing any output at all when using glow to preprocess markdown files.

@Anomalocaridid
Copy link
Contributor Author

Anomalocaridid commented Dec 17, 2023

Marking as draft because I realized my changes cause some of the integration tests to fail. Sorry for not catching it sooner.

@Anomalocaridid Anomalocaridid marked this pull request as ready for review December 18, 2023 04:12
@Anomalocaridid
Copy link
Contributor Author

I fixed the issues with the integration tests.

However, while troubleshooting them, I came across another potential concern that I am unsure how to resolve. I noticed that unlike what I assumed in my initial implementation, less does not appear to send $LESSCLOSE's output to stdout. If I change this to match less's behavior, then that would break the integration tests that test $LESSCLOSE because they rely on bat outputting $LESSCLOSE's output. I am not sure how to change them to work around this, though.

Also, it may be worth seeing at some point if using execute instead of run_script also fixed the issue with the randomly failing integration test.

@Anomalocaridid Anomalocaridid changed the title Refactor lessopen to use execute crate instead of run_script Rework lessopen implementation to use execute crate instead of run_script Dec 18, 2023
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.

None yet

1 participant