Skip to content

remove usage of with context in bitmap_font.py for now #52

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

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

FoamyGuy
Copy link
Contributor

This library will need to be refactored a bit in order to use the with context processor. The specific format Classes like BDF currently assume that the file they are passed will remain open for reading throughout their lifecycle. See #51 for more details.

This change reverts the usage of with context processor in the bitmap_font.py file to get the library back to working for now. We'll plan on doing the refactoring required to go back to the with usage but want to get the library functional in the meantime.

@anecdata if you have a chance give this a try on the device you originally noticed the issue on and confirm whether this branch resolves it for you.

@FoamyGuy FoamyGuy requested review from anecdata and a team November 18, 2021 22:27
Copy link
Contributor

@KeithTheEE KeithTheEE left a comment

Choose a reason for hiding this comment

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

The change looks like it reverts back to the non with usage and should work as a patch before we can refactor the code.

I have a couple of, "How does this work" questions:

Regarding how the magic number is handled in the bdf, pcf, ttf modules: do they restart reading the file from the beginning or do they need the file to be read after the first 4 bytes have been read in?

One last question--I'm not overly familiar with how yield functions and have only just started looking into it, but would yield work in place of undoing the with open? The returned object would be yielded, letting the with structure be preserved to handle file closings in the event that there's an error elsewhere.

If use of yield is a solution, it probably should be it's own pull request, but given this change it feels like a good time to ask about it.

Copy link
Member

@anecdata anecdata left a comment

Choose a reason for hiding this comment

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

Yes, this reverts the file back to like 1.5.1, plus the pylint ignore. This behaves as expected, fonts work. Thanks!

@FoamyGuy
Copy link
Contributor Author

Regarding how the magic number is handled in the bdf, pcf, ttf modules: do they restart reading the file from the beginning or do they need the file to be read after the first 4 bytes have been read in?

I think those classes seek(0) to get it back to the beginning every time they need to read something. But I have not looked through all of them to verify that yet.

One last question--I'm not overly familiar with how yield functions and have only just started looking into it, but would yield work in place of undoing the with open? The returned object would be yielded, letting the with structure be preserved to handle file closings in the event that there's an error elsewhere.

I am also very unfamiliar with yield honestly. If I understand correctly yield allows things to be used comprehension expressions, like for something in my_object: if my_object uses yield it will be capable of having it's contents iterated over in this manner as if it were a List even though it is actually some more complex class. I'm not sure if it could be used in BitmapFont to allow the with processor usage, if so, I don't know how the syntax would look.

@KeithTheEE
Copy link
Contributor

Since we're confident this fixes the issue, I think we can merge it.

I'll give yield a shot this evening, and submit a pull request so we can test it. It might turn out that there's barely any refactoring needed, or it might not work and we have to refactor extensively anyway. If nothing else I'll learn a bit more about when I can and can't use yield

@KeithTheEE KeithTheEE merged commit aeb30d0 into adafruit:main Nov 18, 2021
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Nov 19, 2021
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.

3 participants