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

Idiomatic fixes 2 #5

Merged
merged 21 commits into from
Sep 26, 2023
Merged

Conversation

chris-ha458
Copy link
Contributor

c918f37 and 0b3ff3b need special attention.

Since the length + 1 was not necessary, i removed it and changed some code (length - 1) downstream.

This does affect the match range, and so i changed the range to match the original behavior, but it looks strange now
(0~510 instead of 512?)

other than that it is mostly sideeffectless changes, except 7d65129 which changed exit behavior.

If the original exit code 1 is strictly necessary there are other ways to do recover the behavior as well.

the bounds can be satisfied without both cloned() and copied().
of the two, copied is the more cheaper abstraction so leave only that one.
`or_insert_with` will only call encapsulating function when it is necessary
panic! prints e into stderr, just like stderr.
one difference to the original code would be that it exits with error code 101
if this behavior is not wanted, there's another way to maintain current behavior with idiomatic code.
unless this function is necessary again we can simply replace it
!!!
this changes `intermediary_mean_mess_ratio_calcratio` but this seems more correct?
this is exactly equal to self % rhs
I'm not even sure if this is or the previous is more correct
there are several other ways to do this idiomatically.
we could also use iterators.
@nickspring
Copy link
Owner

nickspring commented Sep 25, 2023

Exit code 1 should be there as I would prefer to have it compatible with Python CLI tool.
But panic! will return exit code > 0 too, yes?

@nickspring
Copy link
Owner

This does affect the match range, and so i changed the range to match the original behavior, but it looks strange now
(0~510 instead of 512?)

what do you mean?

@chris-ha458
Copy link
Contributor Author

What I meant was that the intended range was unclear to me.

@chris-ha458
Copy link
Contributor Author

Exit code 1 should be there as I would prefer to have it compatible with Python CLI tool. But panic! will return exit code > 0 too, yes?

Yes panic! returns 101 as an exit code.
13abf49
is a version that returns specifically 1.

It less concise though.

@nickspring
Copy link
Owner

I guess we can leave panic! here. No problem.

About +1 and 510 - it looks okay.

@chris-ha458
Copy link
Contributor Author

chris-ha458 commented Sep 25, 2023

So which do you prefer?
13abf49
changes it to

let exit_code = match normalizer(&args) {
        Err(e) => {
            eprintln!("{}", e);
            1
        }
        Ok(code) => code,
    };

process::exit(exit_code);

or
as it was before?

match normalizer(&args) {
        Err(e) => panic!("{e}"),
        Ok(exit_code) => process::exit(exit_code),
    }

?

@nickspring
Copy link
Owner

Second one, pls.

@chris-ha458
Copy link
Contributor Author

Reverted!

@nickspring
Copy link
Owner

Is it the final version of PR? Can I check & approve it?

@chris-ha458
Copy link
Contributor Author

Yes. I think other changes can wait until after this one has been considered and merged.

@nickspring nickspring merged commit 8689fe5 into nickspring:main Sep 26, 2023
@chris-ha458 chris-ha458 deleted the idiomatic_fixes_2 branch September 26, 2023 13:38
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