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

feat: Mark builders to be const #172

Merged
merged 2 commits into from
Feb 18, 2025
Merged

feat: Mark builders to be const #172

merged 2 commits into from
Feb 18, 2025

Conversation

Matt3o12
Copy link
Contributor

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
Contributor Author

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? :)

- 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
Owner

Xuanwo commented Dec 18, 2024

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
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
Contributor Author

Matt3o12 commented Feb 18, 2025

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.

Copy link
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
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
6 of 7 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.

2 participants