Skip to content

pr: implement the -e flag#10167

Merged
ChrisDryden merged 3 commits intouutils:mainfrom
cerdelen:pr_e_flag
Feb 14, 2026
Merged

pr: implement the -e flag#10167
ChrisDryden merged 3 commits intouutils:mainfrom
cerdelen:pr_e_flag

Conversation

@cerdelen
Copy link
Copy Markdown
Contributor

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.

@cerdelen
Copy link
Copy Markdown
Contributor Author

@ChrisDryden maybe you could have a look if this looks good to you as you also looked at this issue?

@ChrisDryden
Copy link
Copy Markdown
Collaborator

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?

@cerdelen
Copy link
Copy Markdown
Contributor Author

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.

@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tty/tty-eof (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/tail/follow-name is no longer failing!

@cerdelen cerdelen requested a review from ChrisDryden January 11, 2026 20:25
@cerdelen cerdelen marked this pull request as ready for review January 11, 2026 20:26
@cerdelen
Copy link
Copy Markdown
Contributor Author

Hi, just wanted to ask if there is anything else needed for this to be merged? Thanks in advance.

@ChrisDryden
Copy link
Copy Markdown
Collaborator

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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")) })
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")) })
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

fn split_lines_if_form_feed(file_content: Result<String, std::io::Error>) -> Vec<FileLine> {
fn split_lines_if_form_feed(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cerdelen
Copy link
Copy Markdown
Contributor Author

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

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.

@sylvestre
Copy link
Copy Markdown
Contributor

sorry, it needs to be rebased

fn test_simple_expand_tab() {
let test_cases = vec![
(
"-e",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please generate the files on the fly

i would like to avoid having that many new files in the repo for the tests, thanks

@sylvestre sylvestre changed the title Pr e flag pr: implement the -e flag Feb 11, 2026
@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/pr/bounded-memory. tests/pr/bounded-memory is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/pr/bounded-memory. tests/pr/bounded-memory is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tty/tty-eof (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/factor/t32 is no longer failing!
Congrats! The gnu test tests/factor/t33 is no longer failing!
Note: The gnu test tests/cp/link-heap is now being skipped but was previously passing.
Note: The gnu test tests/seq/seq-epipe is now being skipped but was previously passing.
Congrats! The gnu test tests/printf/printf-surprise is now passing!

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Feb 12, 2026

Merging this PR will not alter performance

✅ 284 untouched benchmarks
⏩ 38 skipped benchmarks1


Comparing cerdelen:pr_e_flag (df212b1) with main (dec633c)

Open in CodSpeed

Footnotes

  1. 38 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions
Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/misc/io-errors. tests/misc/io-errors is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/pr/bounded-memory. tests/pr/bounded-memory is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail/retry. tests/tail/retry is passing on 'main'. Maybe you have to rebase?
Note: The gnu test tests/expand/bounded-memory is now being skipped but was previously passing.
Note: The gnu test tests/rm/many-dir-entries-vs-OOM is now being skipped but was previously passing.
Congrats! The gnu test tests/printf/printf-surprise is now passing!

@ChrisDryden
Copy link
Copy Markdown
Collaborator

I worked on comparing this to all of the GNU PR tests and I did not see any regressions but there were no new passing tests either, but for all of the test cases I checked that used this flag, they had other unrelated issues to the use of this flag. I think even though we can't validate that the end output for those test cases is the same I think we should still go ahead since there's still a bunch of features to add

@ChrisDryden ChrisDryden merged commit 42e3ab6 into uutils:main Feb 14, 2026
155 of 156 checks passed
abendrothj pushed a commit to abendrothj/coreutils that referenced this pull request Feb 17, 2026
* pr: new functionality: expand-tabs + tests

pr: rebase

* pr: pipe test input instead of using files

* pr: parameterize tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants