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

Fix issue where pager would be set to more or most #1494

Merged
merged 4 commits into from
Jul 29, 2023

Conversation

ippsav
Copy link
Contributor

@ippsav ippsav commented Jul 29, 2023

Bug

Colors wouldn't render properly in case the $PAGER is set to more or most.(fix for #1490 )
20230729_23h00m14s_grim

Solution

Handle the case where $PAGER is set to more or most by using bat::config::get_pager_executable which sets the pager to less if that's the case.

Unit tests:

env.rs:

  • test_env_parsing_with_pager_set_to_bat
  • test_env_parsing_with_pager_set_to_more
  • test_env_parsing_with_pager_set_to_most

Copy link
Owner

@dandavison dandavison left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks very much! I've made a couple of small requests for changes to the diff.

src/env.rs Outdated
@@ -41,7 +41,11 @@ impl DeltaEnv {
let pagers = (
env::var(DELTA_PAGER).ok(),
env::var(BAT_PAGER).ok(),
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind getting rid of all explicit mention of BAT_PAGER. get_pager_executable will look at BAT_PAGER itself.

src/env.rs Outdated
@@ -10,7 +10,7 @@ const DELTA_EXPERIMENTAL_MAX_LINE_DISTANCE_FOR_NAIVELY_PAIRED_LINES: &str =
"DELTA_EXPERIMENTAL_MAX_LINE_DISTANCE_FOR_NAIVELY_PAIRED_LINES";
const DELTA_PAGER: &str = "DELTA_PAGER";
const BAT_PAGER: &str = "BAT_PAGER";
const PAGER: &str = "PAGER";
// const PAGER: &str = "PAGER";
Copy link
Owner

Choose a reason for hiding this comment

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

Could you remove this commented line please.

src/env.rs Outdated
@@ -41,7 +41,11 @@ impl DeltaEnv {
let pagers = (
env::var(DELTA_PAGER).ok(),
env::var(BAT_PAGER).ok(),
env::var(PAGER).ok(),
// We're using `bar::config::get_pager_executable` here instead of just returning
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// We're using `bar::config::get_pager_executable` here instead of just returning
// We're using `bat::config::get_pager_executable` here instead of just returning

@ippsav
Copy link
Contributor Author

ippsav commented Jul 29, 2023

@dandavison all fixed ! that's true now that i reread the source code of bat, no need for handling BAT_PAGER, thanks !

@ippsav ippsav requested a review from dandavison July 29, 2023 23:08
@dandavison dandavison merged commit 3c55764 into dandavison:master Jul 29, 2023
12 checks passed
@dandavison
Copy link
Owner

Perfect, thanks for doing this!

@ippsav
Copy link
Contributor Author

ippsav commented Jul 29, 2023

You're welcome ! it was a fun thing to do ! and thanks for making such a useful app !

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