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

Use ruff. add optional different format types to png example #85

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

FoamyGuy
Copy link
Contributor

@FoamyGuy FoamyGuy commented Aug 3, 2024

Changes the repository to use Ruff instead of pylint / black.

Also adds 3 new png image files that have formats with support added by recent PRs and some comented out lines in the png example for the user to test with them if they wish.

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Aug 5, 2024

I am investigating the actions failure. it appears that mpy-cross has taken issue with something.

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Aug 5, 2024

Okay, the root of that issue was that currently used version of mpy-cross (8.2.0) does not support the f-string syntax with !r repr substitutions which ruff wanted to use by default:

f"Unsupported image format {magic_number!r}"

For now we need to use this syntax instead in order to make mpy-cross happy:

"Unsupported image format {!r}".format(magic_number))

It's possible that some future version of mpy-cross would support this in which case we could remove the exclusion from here and use the f-string syntax. I noticed there are newer mpy-cross

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Aug 5, 2024

I tested and confirmed that the f-string syntax does work with mpy-cross 9.x. So, after we make a final 8.x bundle we could switch back to the f-string syntax and remove the exclusion.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

I don't think the actual image files are included. I only see the licenses for them. Everything else looks good. Thanks!

@FoamyGuy
Copy link
Contributor Author

Whoops, thank you. Added the images with the latest commit.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

My browser isn't opening the image files. Is that expected?

@FoamyGuy
Copy link
Contributor Author

Nice catch, I think the image files are corrupted or something. I'll look into more later today and try to get them fixed.

I'm not entirely sure of the cause though. The originals that exist in my local copy of the repo on this branch seem to be functional, I can view them in my OS basic image viewer app. When I download them from Github I end up with files that are different. I compared sha256 sum of one of the images and the downloaded one differs from the one in my local repo. The downloaded one does not render in my OS basic image viewer either.

@FoamyGuy
Copy link
Contributor Author

After some experimentation on another branch and a few tries committing to here I think I've figured this out. The .gitattributes file that was added configuring the eol option seems to be corrupting those PNG files.

I noticed that the broken version of the smallest image was exactly 1 byte smaller than the working version and made a guess that the difference was due to a line ending difference. I'm not exactly sure how to compare the raw bytes within the files but in another branch I tested the theory by removing that file then removing and re-adding the images. After doing that they were working correctly on the Github pages in the browser, and when I download them from Github they also work in my image viewer app. I also confirmed the sha256 hash of the downloaded file now matches the one in my local copy of the repo.

The most recent commits to this branch remove the gitattributes file and then remove and re-add the images. They appear to be functioning correctly for me on this branch now as well.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks! Nice job figuring out the issue.

@tannewt tannewt merged commit 35038a0 into adafruit:main Aug 13, 2024
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 20, 2024
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