icu4x
icu4x copied to clipboard
Document traits as either sealed or implementable
Just like we document structs and enums as either exhaustive or non-exhaustive, we should document traits as either sealed or implementable.
For sealed traits, we could add a __sealed() function, and add a slab like this to the docs:
/// <div class="stab unstable">
/// 🚧 This trait is considered unstable; it may change at any time, in breaking or non-breaking ways,
/// including in SemVer minor releases. Do not implement this trait in userland.
/// </div>
Unclear if there is a Clippy lint to enforce this.
We need to decide both for CldrCalendar specifically and the language/code for the more general case.
Assigned to @Manishearth for the purpose of making a proposal.
For sealing traits I would suggest that we just use the typical : Sealed pattern. We don't need any documentation string because it is impossible to implement outside of our code.
If we need a trait to be implementable across crate boundaries, the Sealed trait can get a doc(hidden) reexport. In such a case we can document using the string above.
For CldrCalendar specifically I think we should seal it and mark every method as doc(hidden) so that nobody calls it; this way it is a pure bound. We should use the unstable doc string above for this trait.
Approval:
- [x] @sffc
- [x] @robertbastian
I like the overall shape of this.
Question: when you say "use the typical : Sealed pattern", what exactly is Sealed? Is it a per-trait type that is defined alongside the trait being sealed? Do we define one per crate in the lib.rs file? And to be clear, it is a trait that is just not exported, which the type system lets us do?
I remain concerned with the proliferation of #[doc(hidden)] for things that are internal/experimental APIs. I apologize for the digression but:
- Sometimes
#[doc(hidden)]means that we want to hide the docs (e.g. for a re-export) and other times#[doc(hidden)]means that we are poking a hole to expose an internal API. I don't really like conflating these two things #[doc(hidden)]doesn't generate any warnings. I wonder if we should be using#[deprecated]instead? Should we be doing that for experimental code, too?- I don't have a problem with people using internal APIs so long as they don't come kicking and screaming that we deleted or changed them. Opt-in is nice, and
#[doc(hidden)]doesn't require opt-in. Should we be putting them in a feature#[cfg(feature = "internal")]or something? - For people whom we trust, it's nice to see the internal APIs in docs. I wonder if we should show them on dev docs and hide them on docs.rs.
For the question in this thread, I am LGTM to make cross-crate-boundary-implementable sealed traits be "marked as internal using a convention which we decide elsewhere", not specifically #[doc(hidden)].
Question: when you say "use the typical
: Sealedpattern", what exactly isSealed? Is it a per-trait type that is defined alongside the trait being sealed? Do we define one per crate in the lib.rs file? And to be clear, it is a trait that is just not exported, which the type system lets us do?
One per crate, does not matter where it lives because it is private.
And yes, it is just not exported.
I don't have a problem with people using internal APIs so long as they don't come kicking and screaming that we deleted or changed them. Opt-in is nice, and #[doc(hidden)] doesn't require opt-in. Should we be putting them in a feature #[cfg(feature = "internal")] or something?
Yeah I think so. Though we have a lot of internal APIs we need to unconditionally make public like provider APIs.
For people whom we trust, it's nice to see the internal APIs in docs. I wonder if we should show them on dev docs and hide them on docs.rs.
Worth considering using document-private-items.
For the question in this thread, I am LGTM to make cross-crate-boundary-implementable sealed traits be "marked as internal using a convention which we decide elsewhere", not specifically #[doc(hidden)].
To be clear, I'm only suggestion doc(hidden) for cases like CldrCalendar where we may want other people to still be able to opt in to it. But I think for now marking CldrCalendar as fully sealed and reopening (hah!) this discussion later when we wish to allow others to reimplement, is a good call.
@sffc quick ping on approving this or moving the discussion forward
Yeah I approve and have already been doing this for example in the icu_pattern code. OK to make the sealed trait be internal according to the decision in https://github.com/unicode-org/icu4x/issues/4467