Fixed PNM bug, housekeeping, cosmetics

Roughsketch / imagesize

Quickly probe the size of various image formats without reading the entire file.

MIT License

57 stars 12 forks source link

Fixed PNM bug, housekeeping, cosmetics #37

Closed virtualritz closed 2 months ago

virtualritz commented 2 months ago

Fixes

Fixed logic bug in PNM format parser (#35).

Housekeeping

Bumped Rust edition to 2021 (ran cargo fix --edition).
Bumped dev-dependencies.

Cosmetics

Ran cargo fmt.
Ran cargo sort.

Roughsketch commented 2 months ago

Thanks for the PR, I'll try to review it later when I have time. For now I'm on my phone so can't see the formatting properly, but I see a few areas where I do not like what cargo fmt has done. I think preferably this PR should fix the issue and I can do a separate fmt pass later.

virtualritz commented 2 months ago

I can offer to fix those either by marking those omitted for rustfmt or by adding a rustfmt.toml with your resp. preferences to this PR.

Roughsketch commented 2 months ago

I generally prefer PRs to have a single purpose to keep things more focused. So ideally this PR should just fix the bug, and there should be another specific PR for formatting that can address these things. I don't like that a very simple fix is being blocked by formatting preferences.

When it comes to format I'm not sure what options there are or if I could craft a file that specifically addresses my issues, but if you give me a bit I can get on my computer and point out a few things I didn't like in this one.

virtualritz commented 2 months ago

Okidoki. So I will add to this PR:

Revert formatting in those places you pointed out and mark them so rustfmt leaves them alone.
Add a rustfmt.toml with a setting for 120 chars line length.
Check is there is a setting that helps alleviate the operator thing you don't like and add that too.

Sounds good?

virtualritz commented 2 months ago

I generally prefer PRs to have a single purpose to keep things more focused.

Totally with you in general.

Specifically though, as a maintainer of several crates myself, I also understand that more granular PRs also mean more work for contributors (it requires branches on the contributor's side then, at the very least).

So the balance for me is single PR within granular commits. This allows the maintainer to cherry pick, if they don't like parts of the PR, so that work isn't wasted.

And when I contribute I try to strike the same balance. Unless I add functionality or a bug fix really requires a lot of code, that is.

I understand if you disagree. The above is just for your consideration. 😊

Roughsketch commented 2 months ago

Okidoki. So I will add to this PR:

* Revert formatting in those places you pointed out and mark them so `rustfmt` leaves them alone.

* Add a `rustfmt.toml` with a setting for 120 chars line length.

* Check is there is a setting that helps alleviate the operator thing you don't like and add that too.

Sounds good?

This sounds good to me, will double check once it's done.

virtualritz commented 2 months ago

Sorry I didn't get to this yet, my last week was super busy.

Roughsketch commented 2 months ago

Sorry I didn't get to this yet, my last week was super busy.

It's all good. I think everything in this PR has been included separately (actual fix was added to #34 and I took the fmt + Cargo changes manually). If you still want to mess with the formatting options feel free, but other than that I think this might be able to be closed as done.