Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cat: Handle all flags correctly #6034

Merged
merged 4 commits into from
Mar 4, 2024

Conversation

BenWiederhake
Copy link
Collaborator

This PR fixes several bugs:

  • Accept repeated flags, just like GNU cat does
  • Make sure that -b overrides -n but not the other way around, just like GNU cat does
  • Accept and ignore the -u flag, just like GNU cat does

This is work towards #5998. Note that this is the first time that args_override_self is correct.

I'm surprised that we already passed the GNU tests before!

@tertsdiepraam
Copy link
Member

I'm thinking now that I was confused with infer_long_args which I added to everything and maybe we didn't do it for args_override_self, because like you already noticed, it takes some effort to check whether it is actually right. Anyway, great work on fixing this up :)

src/uu/cat/src/cat.rs Outdated Show resolved Hide resolved
@BenWiederhake BenWiederhake force-pushed the dev-cat-flags-everything branch from 31e8e3a to cd70d7d Compare March 1, 2024 22:56
@BenWiederhake
Copy link
Collaborator Author

Changes since last push:

  • Removed misleading comment. Thanks @tertsdiepraam!
  • Rebased on current main.

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I'm a bit torn on the test_ prefix. It's good for consistency now, but I'm thinking we could remove the test_ prefix everywhere later. But it's really not important, just something I wanted to bring up at some point.

Should not be a blocker to merge this though.

@cakebaker
Copy link
Contributor

@tertsdiepraam Yes, I agree, though in practice I'm unsure how you would approach it as you can't just remove the prefix.

@cakebaker cakebaker merged commit 4d66af2 into uutils:main Mar 4, 2024
59 of 62 checks passed
@cakebaker
Copy link
Contributor

@BenWiederhake Thanks!

@BenWiederhake BenWiederhake deleted the dev-cat-flags-everything branch March 4, 2024 23:48
@BenWiederhake
Copy link
Collaborator Author

you can't just remove the prefix

I was about to create a new PR removing the prefix everywhere. So let me ask the blunt, but honest question: What is the disadvantage?

@cakebaker
Copy link
Contributor

The "disadvantage" is that you often have to adapt the new function name because something like directory (an example from test_cat.rs) is a bad function name. And with the number of tests we have it's a lot of work.

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