search

JohnDoneth / hd44780-driver

Implementation of the embedded-hal traits for the HD44780.
MIT License
37 stars 40 forks source link

Use enums in set_display_mode #2

Closed sajattack closed 5 years ago

sajattack commented 5 years ago

I would recommend you replace the booleans in set_display_mode with enum variants. Something like 

Display::On
Display::Off
Cursor::Visible
Cursor::Invisible
CursorBlink::On
CursorBlink::Off

This greatly improves readability for code based on your library. When I first looked at your example section, I wondered what 

set_display_mode(true,true,true)

meant, but if you did this it would look like

set_display_mode(Display::On, Cursor::Visible, CursorBlink::On)

I learned this technique watching Rust talks on YouTube. Here are some notes from a talk with more details: https://github.com/killercup/rustfest-idiomatic-libs#avoid-lots-of-booleans

If you think this is a good idea I'd be happy to contribute a PR with these changes.

JohnDoneth commented 5 years ago

Absolutely! I'd be very glad to review a PR with these changes. I agree that is is hard to read what exactly is going on there without the docs on hand.