Explicit LocaleFallbacker argument for DatagenDriver
#5114
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sffc
reviewed
Jun 25, 2024
| #[cfg(feature = "datagen")] | ||
| mod datagen; | ||
| #[cfg(feature = "datagen")] | ||
| pub use datagen::*; |
There was a problem hiding this comment.
Nit: Make sure we bikeshed whether this function is in a module or not with the rest of the WG
Comment on lines
+625
to
+630
| #[test] | ||
| fn test_lookup() { | ||
| assert_eq!(marker_lookup().get("AndListV1Marker"), Some(&Some(icu::list::provider::AndListV1Marker::INFO))); | ||
| assert_eq!(marker_lookup().get("list/and@1"), Some(&Some(icu::list::provider::AndListV1Marker::INFO))); | ||
| assert_eq!(marker_lookup().get("foo"), None); | ||
| } |
There was a problem hiding this comment.
Nit: This test need not be inside the macro?
There was a problem hiding this comment.
I think it's nice to have it next to the function it tests
There was a problem hiding this comment.
I prefer if you move it 15 lines down and out of the macro
Co-authored-by: Shane F. Carr <[email protected]>
sffc
previously approved these changes
Jun 25, 2024
sffc
approved these changes
Jun 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Datagen implicitly requires a
LocaleFallbackerfor most configurations, so I'd rather make it explicit so that providers that don't expose fallback themselves are exportable.I also removed the no-fallback mode, as that can (almost, up to
["und"]) be modeled by the fallback path, and made the required settings arguments tonew.I also added a
supported_markersmethod to theExportableDataProvidertrait; datagen will by default now support all available markers for the provider. This lets me remove theall_markersmethods from the driver crate, which didn't make sense there. This allows removing theall_markers,marker, andmarkersmethods fromicu_datagen, so that it's registry-agnostic.markers_for_binmoves toicu, as that needs theicucrate anyway and only supports markers inicu.