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

fix: seq panic on no arguments #4749 #4750

Merged
merged 4 commits into from
Jul 9, 2023
Merged

Conversation

NikolaiSch
Copy link

This is in reference to issue #4749, where just running seq ends up panicking. I have added a new NoArguments error, and refactored the display formatting, in order to match GNU messages.

@cakebaker cakebaker linked an issue Apr 19, 2023 that may be closed by this pull request
@uutils uutils deleted a comment from github-actions bot Apr 19, 2023
@sylvestre
Copy link
Contributor

please fix the clippy warning and add a test to make sure we don't regress
thanks

@NikolaiSch
Copy link
Author

affirmative. on it asap, expect it within 24 hours!

@NikolaiSch
Copy link
Author

please fix the clippy warning and add a test to make sure we don't regress

thanks

however i have a question, that made clippy fail for me, selinux was not imported correctly for me, but i didn't change any of that code, so what should my next approach be?

@tertsdiepraam
Copy link
Member

I don't understand. Following the clippy lint made clippy fail because of selinux? Or selinux is making clippy fail in general? In the latter case, turning off the feat_selinux feature should help.

@cakebaker
Copy link
Contributor

@NikolaiSch ping?

@github-actions
Copy link

GNU testsuite comparison:

Skipping an intermittent issue tests/tail-2/inotify-dir-recreate

@sylvestre
Copy link
Contributor

I fixed the clippy warnings

@cakebaker
Copy link
Contributor

cakebaker commented Jul 5, 2023

Argh, somehow I made a mess, just wanted to add a test... I added a test to get this PR merged finally.

@tertsdiepraam
Copy link
Member

I think we can merge this, but in my opinion, the Display impl is a bit overengineered. It could just be this:

impl Display for SeqError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::ParseError(s, e) => {
                let error_type = match e {
                    ParseNumberError::Float => "floating point",
                    ParseNumberError::Nan => "'not-a-number'",
                    ParseNumberError::Hex => "hexadecimal",
                };
                write!(f, "invalid {} argument: {}", error_type, s.quote())
            }
            Self::ZeroIncrement(s) => write!(f, "invalid Zero increment value: {}", s.quote()),
            Self::NoArguments => write!(f, "missing operand"),
        }
    }
}

and then the argtype and arg methods could be removed.

Co-authored-by: Terts Diepraam <[email protected]>
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

GNU testsuite comparison:

Skipping an intermittent issue tests/tail-2/inotify-dir-recreate

@cakebaker cakebaker merged commit f8a9552 into uutils:main Jul 9, 2023
@cakebaker
Copy link
Contributor

Thanks :)

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.

seq panics with no arguments
4 participants