add ability to check exit status of command that was run

[tomeichlersmith]

I am making a draft PR because I would want to add some documentation to the README and associated tools, but it appears to be working as intended and so I'd like feedback.

The basic idea is to allow for users to optionally specify an exit code the command should return. This is done by inspecting the command and if the first word starts with -, then the exit code that should be returned comes after.

Examples

Leaving with a non-zero code will do nothing (current behavior).

<!-- cmdrun exit 1 -->

Specifying an exit code that the command returns successfully builds.

<!-- cmdrun -0 some long command that returns 0 if successful -->

Mis-matching exit codes will propagate an error message into the markdown to be rendered.

<!-- cmdrun -0 exit 1 -->

[tomeichlersmith]

@FauconFan I have rebased onto the updated master branch and have implemented "injecting" the error messages into the mdbook with some very simple markdown. I think if the overall design of the code is reasonable then I can work on

• writing a regressions test with a more complete example including stderr and stdout printouts
• write a more meaningful error message, probably taking inspiration from mdbook-admonish which I've used

[FauconFan]

Hi!

I think you are in the right direction!

A few comments and things that comes out of my head:

• Maybe parse_from from clap directly, parsing manually is cool for low-level performance, but I would prefer maintainability here (https://docs.rs/clap/latest/clap/trait.Parser.html#method.parse_from) (this is optional)
• Manage failing edge-cases such as "--expect-return-code a-string-not-a-number", "-NaN", "-42.42", "-"
• Documenting the whole thing within the README file (arguments and error examples)
• Put some regressions and examples within the test book (as you said)
• Error messages can be displayed even though there is an error during parsing, when using your parsing code or clap error

That would be really great overall

I just approved CI in this PR, so you can run your tests within Windows and Linux.

[tomeichlersmith]

Alright, I got clap working (kinda) with a few notable exceptions.

The get_matches_from function requires an iterator which one can naively create using split_whitespace. The problem then is that split_whitespace does not respect quoted substrings and then when I re-join the command and only use one space tests that depend on multiple spaces fail. For example

mdbook-cmdrun/tests/command.rs

Lines 47 to 57 in b3f4248

	add_test!(
	mixed_inline1,
	"yes 42 | head -n 4 | sed -z 's/\\n/ \\n/g'",
	"42 \n42 \n42 \n42", true
	);
	add_test!(
	mixed1,
	"yes 42 | head -n 4 | sed -z 's/\\n/ \\n/g'",
	"42 \n42 \n42 \n42 \n", false
	);

and probably the following on Windoze

mdbook-cmdrun/tests/command.rs

Line 61 in b3f4248

add_test!(simple_inline3, "echo oui non", "oui non", true);

After some googling, I think there are two options forward:

• Only split on the first whitespace. This would then require the flag specifying the desired exit code to be first which isn't a horrible requirement but does put some of the parsing outside of clap. Maybe the index parameter of an Arg would do this as well? I'm not sure.
• Introduce a new dependency for splitting the command string into words for clap like a shell would. e.g. with shellwords or shlex. I like this option better but it would introduce a new dependency which I'm not sure if you are opposed to.

@FauconFan any opinions on these two paths forward?

Edit: The shell word splitting is not without its pitfalls unfortunately. It (I tested with shellwords but it appears the others perform similarly) does not preserve the quote characters which means there are other parsing pitfalls.

[tomeichlersmith]

I guess I was also excited about using clap to make the parsing more maintainable, but these two barriers are making it difficult to use in a idiomatic way.

• We need to pre-split the command string into words to be parsed by clap. This does not preserve the command very naturally without some "pre-parsing" with a shell word splitter and re-introduction of quote characters.
• clap is really opposed to the -N flag. The only solution I could find is allowing negative numbers and then checking if the first positional argument is a negative number. This leads to some "post-parsing" separate from clap.

Maybe after this experimentation we should conclude that using a full CLI parser like clap is not the way forward for this parsing? There is so much parsing "outside" of the parsing done by clap that I don't think using clap is worth it... What do you think @FauconFan ?

It could also certainly be the case that my infamiliarity with clap and Rust is hiding an obvious solution that avoids these issues.

[FauconFan]

Hello @tomeichlersmith,

Sorry for the delay for getting back to you.

I'm not opposed to either shlex or shellwords. I never used them, but they look very stable. For maintainability, I'm more concerned about bug fixing and readability than a few tricks to parse the input in the way we have stated, and if you don't want to use clap, i'm okay with that.

I'm not a huge fan of handcrafting solutions (in this case manual parsing) where it can be done with very established solutions (or if you have a really reason to do it manually!). Another solution would be to not have -N argument and just use clap which is very common in the Rust cli space.

As said, a quick win would be to "drop" the shorthand -N and use clap, and hop it's working
Another would be to switch to another deps. and support the whole thing (but I never worked with these deps so idk)

I'm more interested in the tests that comes with the implementation than the implementation itself (except maintainability). The rest is up to you. Me advising for clap was because I thought that was easy, and apparently it isn't. That's fine

I let you the judge of how you want to proceed.
I'm okay with whatever solution you want to implement, as long as the core feature is implemented and comes with tests and maintainability.

[tomeichlersmith]

That makes sense, I will focus on tests and getting a longer-term maintainable solution.

No worries on the delay :) I am just using this to help learn some Rust so I am not in a huge rush.

[FauconFan]

This is a really good way to learn indeed :)

[tomeichlersmith]

Alright, I dropped back to manual parsing because of the mentioned reasons above. I stored my work on a branch on my fork in case you want to double check that I'm not missing anything obvious. The main issue is that shellwords (and the like) interpret words when splitting causing us to lose some of the escape characters. Using shellwords::escape reintroduces these escape characters but also escapes special characters that were previously not escaped. Instead of adding additional parsing on top of shellwords to make sure the correct characters remain escaped, I choose to drop back to manual parsing and require the flag defining the expected exit code to be the first word. This makes the manual parsing manageable and is not a large requirement.

To Do List

• parse with clap (won't do for above reasons)
• fancier error message like mdbook-admonish (personally not interested in writing necessary html and current solution gets point across)
• handle parsing edge cases and test to make sure
• add regression test for how error messages are displayed
• document new flags with some examples in README