Skip to content
This repository was archived by the owner on May 16, 2023. It is now read-only.

update _write_char #29

Merged
merged 7 commits into from
Jan 17, 2023
Merged

update _write_char #29

merged 7 commits into from
Jan 17, 2023

Conversation

mushunrek
Copy link

Changing the default encoding to cp437, with the possibility to fall back to ascii.

Changing the default encoding to cp437, with the possibility to fall back to ascii.
Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

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

Since this library also supports the microcontrollers, which use UTF-8 encoding and basically ignore this parameter, I think changing the default to something else would confuse users expecting it on those devices - technically, it shouldn't even be ASCII currently.

Additionally, since this is only for the internal _write_char() function, there's no way to modify in a public function like print().

I think a solution could be to provide an encoding argument to print() (and any functions like it) with a default of utf-8 (despite that the printer probably doesn't necessarily support all UTF-8), pass that down to the function you changed, and explain in the public functions that the argument is only supported on Blinka platforms like the Raspberry Pi.

Julian Kern added 2 commits October 24, 2022 10:16
Implementing requested changes.
Specifying encoding in the legacy code
@mushunrek
Copy link
Author

I don't understand exactly why the tests failed...

Important notes: I chose to force the keyword encoding in the function _write_char(...). This lead to a necessary change in thermal_printer_legacy.py, but should not change the user experience.

However, following PEP, I also made the options end and encoding in print(...) keyword-only arguments. This makes code a lot clearer, but may break older codes. If you want the code to be backwards compatible, I can remove this feature.

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

The CI is failing because it wants to reformat the file. You run pre-commit yourself, and then commit the changes it automatically makes.

@tekktrik
Copy link
Member

Looks like we're just missing the note that only UTF-8 is available for CircuitPython, but otherwise looks good!

@tekktrik
Copy link
Member

tekktrik commented Jan 6, 2023

Hi! Looks like there's one remaining change needed, as well as a merge from main needed. Let me know if you're still working on this, otherwise I'm happy to finish it :)

@mushunrek
Copy link
Author

Sorry, this got lost in the other stuff. Added the note.

@tekktrik
Copy link
Member

tekktrik commented Jan 7, 2023

No worries! I'll take a look at this soon :)

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

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

Thanks!

@tekktrik tekktrik merged commit 38c8659 into adafruit:main Jan 17, 2023
@tekktrik tekktrik mentioned this pull request Jan 17, 2023
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants