Closed tranzystorekk closed 2 years ago
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 :)
@niklasmohrin I'd be happy to, just a few issues I can see:
/tmp
directory to verify that a pager was run, if so, I'm guessing it would need to be cleaned up after the test?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 hereWith 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:
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.)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.
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
@tranzystorek-io uh, sorry, you're right. I was thinking of tldr --pager --list
🙈
and I think the
TestEnv::write_config
method doesn't help here
Why is that? I think it should work.
Nevermind, I misunderstood how write_config
works, thought it takes a preexisting config.toml file 😅
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
@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.
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());
}
@tranzystorek-io @dbrgn I guess testing is too difficult for now, let's leave this for later / never
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.)
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?
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.
The enable_styles
flag is part of Config
, which is already passed to print_page
.
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?
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 🙂
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.
The
enable_styles
flag is part ofConfig
, which is already passed toprint_page
.
I can't find it in Config
, in main it seems to come only from the command line args (the --color
flag)
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).
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?
For now, I moved the configure_pager
logic to the output module, as well as rearranged print_page
arguments as discussed
Fixes #169