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

Make MonoTextStyleBuilder fns const #730

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

bugadani
Copy link
Member

@bugadani bugadani commented Aug 9, 2023

Thank you for helping out with embedded-graphics development! Please:

  • Check that you've added passing tests and documentation
  • Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, etc) if your changes affect the public API
  • Run rustfmt on the project
  • Run just build (Linux/macOS only) and make sure it passes. If you use Windows, check that CI passes once you've opened the PR.

PR description

[add your PR description here]

cc #205

@bugadani
Copy link
Member Author

bugadani commented Aug 9, 2023

I guess technically this is a breaking change as the PR doesn't allow nonsense, half-finished builders that have a non-PixelColor type parameter. So technically, the PR can break code that wasn't ever meant to be working anyway, IMO. Any throughts on this?

@taylor-shift
Copy link

Not so sure I can comment on the correctness of this...but it would be great, so we're able to define styles as consts and use them throughout our program instead of having to rebuild them on every call to a display.

@@ -530,6 +524,11 @@ mod tests {
..FONT_6X9
};

const _CAN_BUILD_CONST: MonoTextStyle<'static, BinaryColor> = MonoTextStyleBuilder::new()

Choose a reason for hiding this comment

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

I'd probably move that to a test fn

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be the same thing, just with more lines, and it would also create a test that asserts nothing, and does nothing except print that it passes. Would it really be better?

Choose a reason for hiding this comment

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

personal preference: either test fn or remove entirely - it's just a showcase, right? Either way no strong feelings

@bugadani bugadani force-pushed the const branch 3 times, most recently from 5896a43 to 12b4b28 Compare August 10, 2023 03:17
@bugadani bugadani marked this pull request as ready for review August 10, 2023 03:18
Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I'm ok with the technically breaking change with the new trait bound - it's a good addition to make.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Ah sorry, I was reading through the rest of e-g again and remembered we chose the more verbose style. Needs a cargo fmt if you apply my suggestion too. Don't worry about the CI failure - we're just on a too-old Rust version.

src/mono_font/mono_text_style.rs Outdated Show resolved Hide resolved
@bugadani
Copy link
Member Author

Sorry for not going with your suggestion directly, but there were other bounds to move, too.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

All good! Thanks for the changes.

@jamwaffles jamwaffles merged commit 6524287 into embedded-graphics:master Aug 10, 2023
1 check failed
@bugadani bugadani deleted the const branch August 10, 2023 09:03
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.

None yet

4 participants