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 arguments from PAGER/BAT_PAGER #354

Merged
merged 5 commits into from Oct 17, 2018
Merged

Add arguments from PAGER/BAT_PAGER #354

merged 5 commits into from Oct 17, 2018

Conversation

Foxboron
Copy link
Contributor

I'm a bit unsure of Rust error handling. Probably also missing a test, but lets just ack the code change first maybe.

Solves #352

Signed-off-by: Morten Linderud morten@linderud.pw

@sharkdp
Copy link
Owner

sharkdp commented Oct 14, 2018

Thank you very much for your contribution!

In general, the code-change looks good. It would be great if we could add tests, yes. It's a little bit tricky, because we don't want to rely on the existance of less. Also, we probably don't want to handle any interactive CLI programs in our test suite.

One option could be to write a integration test in the style of

#[test]
fn pager() {
    bat()
        .env("BAT_PAGER", "echo pager-output")
        .arg("test.txt")
        .arg("--paging=always")
        .assert()
        .stdout("pager-output\n")
        .success();
}

There are a few things that we have to test (at least manually):

  • If PAGER=less or BAT_PAGER=less, we need to make sure that the default arguments (--RAW-CONTROL-CHARS, ..) are still passed to less (see is_less logic in the code).
  • The above should also work if we have PAGER=/usr/bin/less, for example.
  • If PAGER="less -X" (or any other arguments), we want to check that the default arguments are not passed to less in addition.
  • Setting PAGER="" should disable the pager.

Concerning the error-handling, I could add that in when the other things are done, if that's okay for you.

@Foxboron
Copy link
Contributor Author

Fixed up on the is_less logic and how it append args. Verified that this gives the desired behavior as described above. Also did a quick swap so the code calling the binary depends on pagerflags. Thus we don't pass /usr/bin/less --flag as the executable when dealing with flags.

src/output.rs Outdated
@@ -28,23 +30,27 @@ impl OutputType {
.or_else(|_| env::var("PAGER"))
.unwrap_or(String::from("less"));

let less_path = PathBuf::from(&pager);
let pagerflags = shell_words::split(&pager).unwrap_or(vec![pager]);
Copy link
Owner

Choose a reason for hiding this comment

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

split will fail if there is a parse error, for example:

split("less --option='foo")   # closing quote is missing

I don't think that unwrap_or would help in such a case?

Copy link
Owner

Choose a reason for hiding this comment

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

Fixed

src/output.rs Outdated
let less_path = PathBuf::from(&pager);
let pagerflags = shell_words::split(&pager).unwrap_or(vec![pager]);

let less_path = PathBuf::from(&pagerflags[0]);
Copy link
Owner

Choose a reason for hiding this comment

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

What happens if we pass BAT_PAGER=""? Does this panic?

Previously, setting BAT_PAGER="" would disable the pager. I'd like to keep this behavior.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, it does.

Copy link
Owner

Choose a reason for hiding this comment

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

Fixed.

src/output.rs Outdated
};

process
.args(&pagerflags[1..])
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. We should make sure that this does not panic.

Copy link
Owner

Choose a reason for hiding this comment

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

Fixed.

Solves sharkdp#352

Signed-off-by: Morten Linderud <morten@linderud.pw>
@sharkdp sharkdp merged commit 2c7087b into sharkdp:master Oct 17, 2018
@Foxboron
Copy link
Contributor Author

Ah, yes. That's a lot neater then the improvements i sat with locally.

@sharkdp
Copy link
Owner

sharkdp commented Oct 17, 2018

Thank you for your work!

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

2 participants