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

Better modularisation in icu_provider #5088

Merged
merged 13 commits into from
Jun 21, 2024

Conversation

robertbastian
Copy link
Member

This PR namesspaces all macros in icu_provider, and exports every type only under a single path (as well as the prelude).

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.

Not convinced this makes things better; I understand the desire to reduce re-exports but then why not make the modules more private? At least the macros are convenient to have on the top level since they are usually called in a chain, whereas the other types can be imported.

#[allow(unused_imports)] // feature-specific
use icu_provider::MaybeSendSync;
use icu_provider::any::MaybeSendSync;
Copy link
Member

Choose a reason for hiding this comment

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

Issue: This trait isn't really any related and should probably be in a different module

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only used for bounds in this module, so the only usages outside this module are AnyPayloadProvider and one tutorial that uses any.

ffi/capi/src/provider.rs Outdated Show resolved Hide resolved
@robertbastian robertbastian requested a review from sffc June 20, 2024 15:43
@robertbastian robertbastian merged commit 100c589 into unicode-org:main Jun 21, 2024
28 checks passed
@robertbastian robertbastian deleted the providerapi branch June 21, 2024 14:35
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