Add environment variable BAT_PAGING

[einfachIrgendwer0815]

Fixes #2629

[Enselic]

Thank you for the PR!

I'm tempted to approve this because of how straightforward and symmetrical it is.

But I don't quite understand what is wrong with the existing method to disable that pager with an env var: BAT_PAGER= bat some-long-file.txt. Could you explain a bit more perhaps?

[einfachIrgendwer0815]

I think setting BAT_PAGER='' to disable paging is a somewhat indirect way of doing so. If you want to disable paging, you want to change the paging behaviour. But by using BAT_PAGER you are actually changing which pager to use. So, correct me if I'm wrong, you're basically saying to bat, "Do paging, but without a pager" which in turn forces bat not to use paging. So BAT_PAGER kind of controls two options here. But saying, "Don't do paging" by setting BAT_PAGING=never would make the intent clearer.

In addition, it is currently not possible to set BAT_PAGER='' to disable paging, but at the same time set a pager to use in case you exceptionally want paging. If there was BAT_PAGING, you could set BAT_PAGING=never to disable paging in general. But you could also set BAT_PAGER to the pager you want when exceptionally using paging. Then when using bat --paging=always, bat would use the pager set by BAT_PAGER.

Also, BAT_PAGING=always allows to set --paging always in the environment, which is currently not possible.

[Enselic]

Makes sense! Would be great if you could a simple regression test for this change. A test that runs with BAT_BAGER set but uses BAT_PAGING=never to disable it. The regression test should fail without the fix. Thanks!

[einfachIrgendwer0815]

I have just come across this:

bat/tests/utils/command.rs

Lines 6 to 21 in 6dd8b28

	pub fn bat_raw_command_with_config() -> Command {
	let mut cmd = Command::cargo_bin("bat").unwrap();
	cmd.current_dir("tests/examples");
	cmd.env_remove("BAT_CACHE_PATH");
	cmd.env_remove("BAT_CONFIG_DIR");
	cmd.env_remove("BAT_CONFIG_PATH");
	cmd.env_remove("BAT_OPTS");
	cmd.env_remove("BAT_PAGER");
	cmd.env_remove("BAT_STYLE");
	cmd.env_remove("BAT_TABS");
	cmd.env_remove("BAT_THEME");
	cmd.env_remove("COLORTERM");
	cmd.env_remove("NO_COLOR");
	cmd.env_remove("PAGER");
	cmd
	}

bat/tests/benchmarks/run-benchmarks.sh

Lines 32 to 43 in 6dd8b28

	# Clean up environment
	unset BAT_CACHE_PATH
	unset BAT_CONFIG_DIR
	unset BAT_CONFIG_PATH
	unset BAT_OPTS
	unset BAT_PAGER
	unset BAT_STYLE
	unset BAT_TABS
	unset BAT_THEME
	unset COLORTERM
	unset NO_COLOR
	unset PAGER

bat/tests/syntax-tests/create_highlighted_versions.py

Lines 41 to 53 in 6dd8b28

	def create_highlighted_version(args):
	output_basepath, source = args
	env = os.environ.copy()
	env.pop("BAT_CACHE_PATH", None)
	env.pop("BAT_CONFIG_DIR", None)
	env.pop("BAT_CONFIG_PATH", None)
	env.pop("BAT_OPTS", None)
	env.pop("BAT_PAGER", None)
	env.pop("BAT_STYLE", None)
	env.pop("BAT_TABS", None)
	env.pop("BAT_THEME", None)
	env.pop("NO_COLOR", None)
	env.pop("PAGER", None)

Should BAT_PAGING be added?

[Enselic]

You'll want to add a test to https://github.com/sharkdp/bat/blob/master/tests/integration_tests.rs, search for BAT_PAGER in that file

[einfachIrgendwer0815]

Makes sense! Would be great if you could a simple regression test for this change. A test that runs with BAT_BAGER set but uses BAT_PAGING=never to disable it. The regression test should fail without the fix. Thanks!

Unfortunately, that test doesn't fail without the fix applied because the tests don't run bat on an interactive terminal, which means the default is no paging, so setting BAT_PAGING=never wouldn't change anything.

So instead the test I wrote sets BAT_PAGING=always to make sure BAT_PAGING actually has an effect.

[Enselic]

Thank you for the effort. Let's merge.