Don't add artificial newline to last line if --style=plain

[Enselic]

This fixes #1438.

Note however, that using a pager such as less will add a newline itself.
So to actually not print a newline for such files, you need to either
disable paging:

bat --style=plain --paging=never no-newline-at-end-of-file.txt

or use a "pager" that does not add a newline:

bat --style=plain --pager=cat no-newline-at-end-of-file.txt

[Enselic]

I'd say the fix definitely should include some integration tests too, but before I put too much time into that, I just wanted to check if this fix looks reasonable to you?

[sharkdp]

Thank you for your contribution!

I'd say the fix definitely should include some integration tests too, but before I put too much time into that, I just wanted to check if this fix looks reasonable to you?

It does! :-)

The "Test with new syntaxes and themes" currently fails in the syntax regression tests, as there are some files without newlines at the end. This should be fixed anyway (trailing newlines should be added).

[Enselic]

Just a quick status update. I've pushed a set of commits that (should) pass all existing tests. Feel free to code-review if you wish.

However, to add new tests for this usecase, I need to come up with a way to test tty output.

From what I can tell, only non-tty output is tested in tests/integration_tests.rs. And after a quick glance at the assert_cmd crate, tty output/simulation does not seem to be not supported.

So it seems to me as if the right course of action is to add support for tty-output-testing to assert_cmd, bump our dependency, and then add tests to this fix. So it will take a while to get in place.

If you have idea for other approaches, I am all ears.

[sharkdp]

If you have idea for other approaches, I am all ears.

I don't think we need to mock a complete terminal to test this. We can force bat to run in "interactive mode" (as if it was connected to a terminal) by using either --color=always or --decorations=always.

I added a test that fails on master but succeeds on your branch.

The changes here look great - thank you very much!

[Enselic]

Thanks a lot for the help with adding a test for this fix!

I found no dedicated regression test for the fix for #299, so I added such a test in PR #1445. The new test fails if the fix for #299 is reverted from master, but of course pass on unmodified master.

It is true that we can force bat options to behave as if a tty was used, but I still think tests for tty defaults has some merit. There is currently no test that fails if we mess up the code paths that depends on e.g. atty::is(Stream::Stdout) being true, though such regressions arguable would affect a lot of users, and the involved code is rather complex. Making such tests possible to write in a maintainable manner is a big topic of its own, so let's defer that discussion to some other occasion though :)

[sharkdp]

I found no dedicated regression test for the fix for #299, so I added such a test in PR #1445. The new test fails if the fix for #299 is reverted from master, but of course pass on unmodified master.

👍

It is true that we can force bat options to behave as if a tty was used, but I still think tests for tty defaults has some merit. There is currently no test that fails if we mess up the code paths that depends on e.g. atty::is(Stream::Stdout) being true, though such regressions arguable would affect a lot of users, and the involved code is rather complex.

Interesting thoughts. I agree in general. It would be great to see an explicit example where such a test would be superior over forcing colorized/decorated output.. even if constructed.

Making such tests possible to write in a maintainable manner is a big topic of its own, so let's defer that discussion to some other occasion though :)

Yes, I think that should be discussed in a new ticket 👍