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

Returning ExtraneousLocale in BlobDataProvider #3562

Merged
merged 3 commits into from
Jun 23, 2023

Conversation

robertbastian
Copy link
Member

@robertbastian robertbastian commented Jun 22, 2023

And not in datagen

Fixes #3505

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

Technically speaking, both BlobDataProvider and DatagenProvider should be returning the ExtraneousLocale error. We're not doing it consistently everywhere in DatagenProvider. But I also understand that it's hard to enforce consistently across all the various impls of DatagenProvider.

@Manishearth
Copy link
Member

In favor of the blob changes, feel iffy about the datagen ones; I don't mind having those around.

@robertbastian
Copy link
Member Author

I removed the datagen ones because only a handful out of dozens of singleton implementations correctly return ExtraneousLocale. Currently the datagen provider's error cases aren't great across the board, for example if you call a non-singleton load with a locale that's not in supported_locales, you'll often get an IO error instead of a MissingLocale error. I'm happy to fix that separately if we feel like that's an issue.

@robertbastian robertbastian requested a review from sffc June 23, 2023 08:26
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

seems fair. would like the defence in depth of having these errors in more places, but not particularly insistent

@robertbastian robertbastian merged commit 8b30bb1 into unicode-org:main Jun 23, 2023
23 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.

BlobDataProvider should return DataErrorKind::ExtraneousLocale for singleton structs
3 participants