pr: implement the -e flag #10167
pr: implement the -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. |
|
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
src/uu/pr/src/pr.rs
Outdated
| } | ||
|
|
||
| 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. |
|
sorry, it needs to be rebased |
| fn test_simple_expand_tab() { | ||
| let test_cases = vec
There was a problem hiding this comment.
please generate the files on the fly
i would like to avoid having that many new files in the repo for the tests, thanks
| pr-error-column-merge-conflict = cannot specify number of columns when printing in parallel | ||
| pr-error-across-merge-conflict = cannot specify both printing across and printing in parallel | ||
| pr-error-invalid-pages-range = invalid --pages argument '{$start}:{$end}' | ||
| pr-error-invalid-expand-tab-argument='-e' extra characters or invalid number in the argument: ‘{$arg}’ |
There was a problem hiding this comment.
| pr-error-invalid-expand-tab-argument='-e' extra characters or invalid number in the argument: ‘{$arg}’ | |
| pr-error-invalid-expand-tab-argument = '-e' extra characters or invalid number in the argument: ‘{$arg}’ |
| pr-error-column-merge-conflict = impossible de spécifier le nombre de colonnes lors de l'impression en parallèle | ||
| pr-error-across-merge-conflict = impossible de spécifier à la fois l'impression transversale et l'impression en parallèle | ||
| pr-error-invalid-pages-range = argument --pages invalide '{$start}:{$end}' | ||
| pr-error-invalid-expand-tab-argument= Caractères supplémentaires ou nombre invalide dans l'argument de '-e': '{$arg}' |
There was a problem hiding this comment.
| pr-error-invalid-expand-tab-argument= Caractères supplémentaires ou nombre invalide dans l'argument de '-e': '{$arg}' | |
| pr-error-invalid-expand-tab-argument = Caractères supplémentaires ou nombre invalide dans l'argument de '-e': '{$arg}' |
|
GNU testsuite comparison: |
3 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.