Move pager configuration near render logic

[tranzystorekk]

Fixes #169

[niklasmohrin]

Could you write a test so that the discussed refactor later doesn't break this again? I think it would be really cool if you mocked the pager with some shell script that just writes a file to a tempdir when called. Then you would just have to set the script as the pager through the environment variable. If you don't want to, we can also open a new issue for someone else to write the test though :)

[tranzystorekk]

@niklasmohrin I'd be happy to, just a few issues I can see:

• I guess you mean writing an empty file to the /tmp directory to verify that a pager was run, if so, I'm guessing it would need to be cleaned up after the test?
• With the new clap-derive parsing, I cannot do tldr --pager --update without giving a command, I'd have to somehow modify the display.use_pager config in the toml file, and I think the TestEnv::write_config method doesn't help here

[dbrgn]

With the new clap-derive parsing, I cannot do, tldr --pager --update without giving a command

Hm, that actually sounds like a useful combination though. If you want, you could change that behavior in a separate PR:

• Create a new ArgGroup similar to command_or_file called action. It would depend on &["command", "render", "list"]. (Maybe it can also depend on &["command_or_file", "list"] instead, I'm not sure if that works.)
• The pager could then be changed from requires = "command_or_file" to requires = "action".

I guess you mean writing an empty file to the /tmp directory to verify that a pager was run, if so, I'm guessing it would need to be cleaned_up after the test?

I'm not sure what @niklasmohrin had in mind, but cleanup sounds like a good thing. The thing is that it'll be a bit tricky. You cannot write the file to a static path, because then multiple tests would overwrite the file. And if you use a random path, you need some way to communicate that path between the test and the mock pager.

Ideally you would somehow be able to pass a filepath from the test to the mock pager. Then the test could reuse the TempDir struct that's already being used in TestEnv, which features auto-cleanup after the test is done.

Oh, and the mock pager script doesn't necessarily need to be a hard-to-port shell script, it could be a new Rust crate in a subdirectory as well (tests/mock_pager/). The tests would need to have a dev-dependency on that sub-crate though, so that it's always built first.

[tranzystorekk]

Hm, that actually sounds like a useful combination though.

I'm not sure what you have in mind, when we just want to run an update, a pager seems completely unnecessary anyway

[dbrgn]

@tranzystorek-io uh, sorry, you're right. I was thinking of tldr --pager --list 🙈

[dbrgn]

and I think the TestEnv::write_config method doesn't help here

Why is that? I think it should work.

[tranzystorekk]

Nevermind, I misunderstood how write_config works, thought it takes a preexisting config.toml file 😅

[tranzystorekk]

The tests would need to have a dev-dependency on that sub-crate though, so that it's always built first.

AFAIK a binary crate cannot be a dependency, unfortunately

[dbrgn]

@tranzystorek-io hm, you're right. you can have both a bin.rs and a lib.rs in your crate, but if you add a dependency, then only the lib part will be built.

[niklasmohrin]

My initial idea was to have a short script that creates a file (it would be even easier with PAGER="touch testenvcache/tealdeer_log.txt) which would be a very easy check whether or not the pager was run. I think it would be fine to not have a super portable test for this because we only want to check when a pager is spawned, which should behave the same on all platforms. The problem is that the pager crate by default checks whether stdout is a tty and doesn't spawn the pager if not - as it is in when running subprocesses in tests. I found a way to have it use the stdout from the tests - that means it closes the file descriptor on exit though :D We could probably make some dup(2) magic if we want to follow this path, but it is a bit hacky already. For reference, this is my final result:

#[test]
fn test_pager_is_called() {
    let testenv = TestEnv::new();
    testenv.add_entry("thecommand", "thecontent");
    let logfile = testenv.cache_dir.path().join("tealdeer_log.txt");

    assert!(!logfile.exists());
    testenv
        .command()
        .env("PAGER", format!("touch {}", logfile.display()))
        .arg("--pager") // <- Comment and uncomment this line
        .arg("thecommand")
        .stdout(unsafe { std::process::Stdio::from_raw_fd(std::io::stdout().as_raw_fd()) })
        .assert()
        .success();
    assert!(logfile.exists());
}

[niklasmohrin]

@tranzystorek-io @dbrgn I guess testing is too difficult for now, let's leave this for later _{/ never}

[dbrgn]

Since both calls are directly followed by print_page, and since we want to use the pager only for printing the page, wouldn't it be better to set up the pager in print_page? (It has access to the config already.)

[niklasmohrin]

Sounds like a good idea, we should probably make sure to a) check whether we can call configure_pager multiple times or wrap it in a Once or so? (I wouldn't expect print_page to have the contract we would otherwise attach to it). What do you think?

[tranzystorekk]

One thing I'm worrying is that configure_pager needs the enable_styles flag, so it would need to be passed to print_page, which seems like a separate, unrelated concern.

[dbrgn]

The enable_styles flag is part of Config, which is already passed to print_page.

[niklasmohrin]

I agree that (even without the proposed addition) print_page does a lot of things, I suppose the enable_markdown branching is more similar to the pager thing than it is to actually printing to the terminal. Maybe we should move the two printing blocks into separate "functionality" functions and leave all of the rest (pager, markdown) in one "business logic" function. What do you think?

[dbrgn]

I don't really see an issue with print_page, the function deals with outputting the page in the proper mode to the terminal, with or without markdown, with or without pager. Locking stdout is also "preparation logic", but it clearly has to do with "page printing".

However, if you feel this could be improved, feel free to propose an improvement in a PR 🙂

[dbrgn]

In any case, the goal here is to fix #169, and moving the pager setup logic into print_page would solve this. Further improvements can be done later.

[tranzystorekk]

The enable_styles flag is part of Config, which is already passed to print_page.

I can't find it in Config, in main it seems to come only from the command line args (the --color flag)

[dbrgn]

Uhh, sorry @tranzystorek-io, you're right of course (I was mixing up config.display.use_pager and enable_styles).

Maybe we should handle styles for printing error messages differently instead of passing around the flag everywhere (maybe a global static or thread-local variable?). However, for now I'd say passing it to print_page is OK (better than repeating the configure_pager call twice).

[niklasmohrin]

Maybe we should handle styles for printing error messages differently instead of passing around the flag everywhere (maybe a global static or thread-local variable?).

Didn't we discuss an OutputManager or something similar at some point?

[tranzystorekk]

For now, I moved the configure_pager logic to the output module, as well as rearranged print_page arguments as discussed

[dbrgn]

One last change request.

[niklasmohrin]

Very nice!

I also ran a quick benchmark, Once doesn't seem to make any difference 👍

[dbrgn]

Thanks!