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

Fixed PNM bug, housekeeping, cosmetics #37

Closed
wants to merge 3 commits into from

Conversation

virtualritz
Copy link

Fixes

Housekeeping

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

Cosmetics

  • Ran cargo fmt.
  • Ran cargo sort.

@Roughsketch
Copy link
Owner

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
Copy link
Author

virtualritz commented Jun 8, 2024

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
Copy link
Owner

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.

Copy link
Owner

@Roughsketch Roughsketch left a comment

Choose a reason for hiding this comment

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

These are probably the main places that bother me. I didn't tag every instance, but basically every byte block gets messed up in similar ways.

As for minor things I don't personally love but am probably fine with keeping if it's automated:

  • Assert blocks going into multiple lines
    • I think they're neat in single but I can see how it may be easier to read like this
  • Some function definitions get multi-lined
    • I know this is column based, but I think the default split is only 80?
    • Would 120 be too much?
  • Long logic in if statement puts operators on left
    • Probably easier to read but not my preference

b"MA1B",
b"avci", b"avcs", b"heic", b"heim", b"heis", b"heix", b"hevc", b"hevm", b"hevs", b"hevx",
b"jpeg", b"jpgs", b"mif1", b"msf1", b"mif2", b"pred", // AVIF specific
b"avif", b"avio", b"avis", b"MA1A", b"MA1B",
Copy link
Owner

Choose a reason for hiding this comment

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

The comments here for HEIF specific and AVIF specific get messed up.

0x10, 0x20, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, // Image 2 (10x100)
0x0A, 0x64, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, // Image 3 (255x? run out of bytes)
Copy link
Owner

Choose a reason for hiding this comment

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

Another instance where comments were intended for a specific layout get messed up

0x00, 0x00, 0x00, 0x7B, 0x00, 0x00, 0x01, 0x41,
0x08, 0x06, 0x00, 0x00, 0x00, 0x9A, 0x38, 0xC4];
let data = vec![
0x89, 0x51, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, 0x44,
Copy link
Owner

Choose a reason for hiding this comment

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

Not a fan of how many it's deciding to put on a single line here because it makes it harder to track things in 4 byte increments. Previous was 8 per row, but this is 15.

@virtualritz
Copy link
Author

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
Copy link
Author

virtualritz commented Jun 9, 2024

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
Copy link
Owner

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
Copy link
Author

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

@Roughsketch
Copy link
Owner

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.

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