Pr e flag #10167
Pr e flag #10167
Conversation
|
@ChrisDryden maybe you could have a look if this looks good to you as you also looked at this issue? |
|
Can you add some integration tests for the cases that we talked about in the other PR? not to validate the correct output but high level things like it not consuming the file afterwards and working for both the short form and the long form? |
|
Yes i will do that tomorrow (its late here already). Also im almost certain i expand the chars with +1 too much but thats why this is a draft and i was just wanting to have someone look over it from a high level. |
The 0 guard is unnecessary because the result is always at least 1
Turns out GNU pr expands the input char AND tabs
Turns out tabs are getting expanded with the default value (8) if both parameters are given.
|
GNU testsuite comparison: |
|
Hi, just wanted to ask if there is anything else needed for this to be merged? Thanks in advance. |
|
Hey my bad that it took a while to get to this. When I was thinking of tests I was thinking it would be better to have a few very basic ones instead of that comprehensive test suite you have, for the main reason that the current implementation of pr has many different missing things from the GNU implementation that it would probably be better to just add the basic tests that recognize that the values are not throwing an error and that they pass very basic tests that show that the value is being parsed, with a comment saying that more tests need to be added when the pr outputs are more closely matching the GNU pr implementation Another way to view this is to look at the output of PR for the test cases you have currently and you will see that the GNU pr utility will product a very different output, so it would be better to wait until we are far enough along with this utility so that our integ tests test that it actually matches the output of the GNU implementation |
| # Page header text | ||
| pr-page = Page | ||
| pr-try-help-message = Try 'pr --help' for more information. |
There was a problem hiding this comment.
This message in other utilities we get from using the UError from uucore, it just wraps the error message with this for each utility. For example this error message: https://github.com/uutils/coreutils/blob/main/src/uu/stty/src/stty.rs#L447 is wrapped with: "Try "stty --help" for more imformation."
| } else if s.len() > 1 { | ||
| s[1..] | ||
| .parse() | ||
| .map_err(|_e| PrError::EncounteredErrors { msg: format!("{}\n{}", translate!("pr-error-invalid-expand-tab-argument", "arg" => &s[1..]), translate!("pr-try-help-message")) }) |
There was a problem hiding this comment.
Same comment as here: https://github.com/uutils/coreutils/pull/10167/changes#r2688579349
| if c.is_ascii_digit() { | ||
| s | ||
| .parse() | ||
| .map_err(|_e| PrError::EncounteredErrors { msg: format!("{}\n{}", translate!("pr-error-invalid-expand-tab-argument", "arg" => s), translate!("pr-try-help-message")) }) |
There was a problem hiding this comment.
Same comment as here: https://github.com/uutils/coreutils/pull/10167/changes#r2688579349
| } | ||
|
|
||
| fn split_lines_if_form_feed(file_content: Result<String, std::io::Error>) -> Vec<FileLine> { | ||
| fn split_lines_if_form_feed( |
There was a problem hiding this comment.
This is a bit tricky to review from the context that when trying to see if the implementation of -e is correct I don't have the baseline that the other aspects of the pr utility is working correctly.
I would personally just try to implement the handler first and then making sure that the base case for pr actually produces the same output as pr before implementing the logic for the many flags and options that can be provided.
Could you tell me which ones are having different output? I created the expected files with the GNU pr and the flags i wanted to push so it should not produce different outputs. I also crossreferenced them on Mac as well as on Arch with the GNU utils so i would be sure that these are the outputs that GNU produces. |
2 participants
In Issue: #10100 the abscence of -e is raised. This Pr draft is the parsing as well as the implementation.
Tests are missing so far.