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

Add derive macro for BuilderLite, add #[non_exhaustive] to some enums and structs #2614

Merged
merged 9 commits into from
Nov 27, 2024

Conversation

jessebraham
Copy link
Member

This is working towards #2500.

First, I've created a derive procmacro to implement the Builder Lite pattern on a struct automatically. Definitely not an expert on procmacros, so welcome to hear any feedback here, but seems to be working at least.

Additionally, I've started adding #[non_exhaustive] to public enums and structs where necessary, starting with the core peripherals which we are looking to stabilize in our 1.x.x. release. Based on comments in the GPIO analysis issue, it is not clear that any of these are currently required there, but let me know if this is incorrect. Additionally, if you feel any of these attributes are missing in the included drivers, or that any which I've added are not necessary, please let me know.

Once this is merged I will continue to work on adding these to other drivers, just wanted to prove this out first with a limited subset.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 26, 2024

Very nice!

Just the generated docs look a bit off:

// Recursive expansion of procmacros::BuilderLite macro
// =====================================================

#[automatically_derived]
impl Config {
    #[doc = r" Assign the given value to the `#field_ident` field."]
    pub fn with_frequency(mut self, frequency: HertzU32) -> Self {
        self.frequency = frequency;
        self
    }
    #[doc = r" Assign the given value to the `#field_ident` field."]
    pub fn with_timeout(mut self, timeout: u32) -> Self {
        self.timeout = Some(timeout);
        self
    }
}

@jessebraham
Copy link
Member Author

Ahh oops, guess that didn't work how I expected it to 😅 Will fix, thanks

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

Thanks!

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 27, 2024

Maybe this PR could also apply it to esp_hal::Config but we can do that later

@MabezDev MabezDev added this pull request to the merge queue Nov 27, 2024
Merged via the queue into esp-rs:main with commit 1a2bee6 Nov 27, 2024
28 checks passed
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.

4 participants