-
Notifications
You must be signed in to change notification settings - Fork 176
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
Use Option<T> over FFI #5485
Use Option<T> over FFI #5485
Conversation
b3a4851
to
95896d1
Compare
9daa508
to
faa1dd9
Compare
Ready for review. I think this is the last large part of FFI future-proofing. |
faa1dd9
to
29be928
Compare
@@ -77,7 +70,7 @@ pub mod ffi { | |||
secondary_group_size: u8, | |||
min_group_size: u8, | |||
digits: &[DiplomatChar], | |||
grouping_strategy: FixedDecimalGroupingStrategy, | |||
grouping_strategy: Option<FixedDecimalGroupingStrategy>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's inconsistent to not have an options bag on some APIs, when the Rust API takes an options bag. Everywhere you call Options::default()
in a constructor it should really take a bag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, perhaps we should tweak this
No description provided.