nickspring / charset-normalizer-rs

Truly universal encoding detector in pure Rust - port of Python version
https://crates.io/crates/charset-normalizer-rs
MIT License
30 stars 3 forks source link

Idiomatic fixes 2 #5

Closed chris-ha458 closed 1 year ago

chris-ha458 commented 1 year ago

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.

nickspring commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

chris-ha458 commented 1 year ago

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 commented 1 year ago

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

About +1 and 510 - it looks okay.

chris-ha458 commented 1 year ago

So which do you prefer? 13abf49fb4f2b7fdcd1b0316b244ef4cbbbbbbba
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 commented 1 year ago

Second one, pls.

chris-ha458 commented 1 year ago

Reverted!

nickspring commented 1 year ago

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

chris-ha458 commented 1 year ago

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