Skip to content

feat: Mark builders to be const#172

Merged
Xuanwo merged 2 commits intoXuanwo:mainfrom
Matt3o12:main
Feb 18, 2025
Merged

feat: Mark builders to be const#172
Xuanwo merged 2 commits intoXuanwo:mainfrom
Matt3o12:main

Conversation

@Matt3o12
Copy link
Copy Markdown
Contributor

@Matt3o12 Matt3o12 commented Dec 17, 2024

Hello thank you for your work.

I have made changes so that all builders are now const. They should still be 100% backwards compadible. I had to introduce a constructor, new, as Default cannot be const just yet.

This allows me to define a const builder at the top and use it like this:

    const BACKOFF_BUILDER: ExponentialBuilder = ExponentialBuilder::new().with_min_delay(Duration::from_millis(1));

    fn test() {
        my_fn().retry(BACKOFF_BUILDER).call();
    }

All changes:

  • Refactored all methods inside FibonacciBuilder, ExponentialBuilder, and ConstantBuilder to be const.
  • Added a new const new constructor for each builder which uses the values from default()
  • Updated the default method to call new().
  • Added unit tests for the const builders to ensure they have the correct values.

@Matt3o12
Copy link
Copy Markdown
Contributor Author

Matt3o12 commented Dec 17, 2024

It seems like debug_asserts are causing problems with the MSRV:

    /// Set the factor for the backoff.
    ///
    /// # Panics
    ///
    /// This function will panic if the input factor is less than `1.0`.
    pub const fn with_factor(mut self, factor: f32) -> Self {
        debug_assert!(factor >= 1.0, "invalid factor that lower than 1");

        self.factor = factor;
        self
    }

When do you plan to bump the MSRV or do you have an idea how to fix that in the meantime? :)

@Matt3o12
Refactor builders to be const
eda1345 
- Refactored all methods inside FibonacciBuilder, ExponentialBuilder, and ConstantBuilder to be const.
- Added a new const `new` constructor for each builder which uses the values from default()
- Updated the default method to call `new()`.
- Added unit tests for the const builders to ensure they have the correct values.
@Xuanwo
Copy link
Copy Markdown
Owner

Xuanwo commented Dec 18, 2024 •
edited
Loading

Hi @Matt3o12, thank you for your work first! I believe we can remove that debug_assert. Would you like to update the documentation for with_factor to emphasize the importance of ensuring that factor >= 1?

@Xuanwo
Copy link
Copy Markdown
Owner

Xuanwo commented Feb 18, 2025

Hi @Matt3o12, are you interested in moving this PR forward?

@Xuanwo Xuanwo changed the title Refactor builders to be const feat: Mark builders to be const Feb 18, 2025
@Matt3o12
Copy link
Copy Markdown
Contributor Author

Matt3o12 commented Feb 18, 2025 •
edited
Loading

Hey, sorry about the late reply, I have forgotten about it. I have now removed the debug_assert and updated the docs. Please let me know if you need me to change anything or to roll up the commits.

Edit: it seems you have to manually rerun the windows job or maybe empty the cache.

Xuanwo
Xuanwo approved these changes Feb 18, 2025
View reviewed changes
Copy link
Copy Markdown
Owner

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @Matt3o12 for working on this!

@Xuanwo
Copy link
Copy Markdown
Owner

Xuanwo commented Feb 18, 2025

Edit: it seems you have to manually rerun the windows job or maybe empty the cache.

It's not related to this PR. embassy-time seems not work correctly on windows.

@Xuanwo Xuanwo merged commit a1351c2 into Xuanwo:main Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Matt3o12 @Xuanwo