Skip to content

chgrp: use map instead of match in parse_gid_and_uid() - #13620

Open
Devel08 wants to merge 7 commits into
uutils:mainfrom
Devel08:chgrp-use-map
Open

chgrp: use map instead of match in parse_gid_and_uid()#13620
Devel08 wants to merge 7 commits into
uutils:mainfrom
Devel08:chgrp-use-map

Conversation

@Devel08

@Devel08 Devel08 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/date/resolution (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/retry (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/tail/tail-n0f is now passing!

@xtqqczze

Copy link
Copy Markdown
Contributor

Instead of formatting the error inline, we could follow the pattern used by other utils and introduce the thiserror crate.

That would let us move the error definitions into an enum, e.g.:

#[derive(Error, Debug)]
enum ChgrpError {
    #[error("{}", translate!("chgrp-error-invalid-user", "from_group" => .from_group))]
    InvalidUser { from_group: String },
}

Comment thread src/uu/chgrp/src/chgrp.rs Outdated
InvalidUser { from_group: String },
}

impl UError for ChgrpError {

@xtqqczze xtqqczze Jul 29, 2026

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.

The default implementation will already use an exit code of 1:

impl UError for ChgrpError {}

Comment thread src/uu/chgrp/Cargo.toml Outdated
clap = { workspace = true }
uucore = { workspace = true, features = ["entries", "fs", "perms"] }
fluent = { workspace = true }
thiserror = { workspace = true }

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.

nit: can this go between clap and uucore?

At some point I want to alphabetise these sections and that will minimise the diff.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

alr

@xtqqczze

Copy link
Copy Markdown
Contributor

LGTM

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.

2 participants