Concept: unstable feature #6629
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
robertbastian
force-pushed
the
unstable
branch
4 times, most recently
from
7d010b5 to
b7b7896
Compare
There was a problem hiding this comment.
Generally in favor of the new feature, let's discuss how this gets surfaced to the user in #6630 (review)
| pub trait IntoAnyCalendar: Calendar + Sized { | ||
| /// Convert this calendar into an [`AnyCalendar`], moving it | ||
| /// | ||
| /// You should not need to call this method directly | ||
| #[cfg_attr(not(feature = "unstable"), doc(hidden))] |
There was a problem hiding this comment.
thought: is this a breaking change? These APIs were publicly exposed even though this trait was sealed (they can be called if not used)
There was a problem hiding this comment.
or, well, marking things as hidden is not a breaking change, but marking things as hidden with the understanding that they may change in the future is.
There was a problem hiding this comment.
In retrospect we probably should have doc(hidden)'d these methods (and those of Calendar)
There was a problem hiding this comment.
Hmm, I hadn't considered these functions to be unstable. I expect client code to be able to use the functions on the Calendar and IntoAnyCalendar traits in a stable way.
There was a problem hiding this comment.
I'm not opposed to them being stable; I was mostly thinking of these as "partially internal" where they could go either way. I've definitely had use for IntoAnyCalendar.
"Can be called but not implemented" is a valid use of sealed traits.
components/calendar/src/calendar.rs
Outdated
| @@ -26,14 +26,17 @@ use core::fmt; | |||
| /// </div> | |||
| pub trait Calendar: crate::cal::scaffold::UnstableSealed { | |||
| /// The internal type used to represent dates | |||
| #[cfg_attr(not(feature = "unstable"), doc(hidden))] | |||
There was a problem hiding this comment.
thought: ditto on stability here
There was a problem hiding this comment.
I am not currently convinced that the types and methods on Date should be marked unstable.
There was a problem hiding this comment.
please comment on the APIs you're referring to
| /// 🚧 This method is considered unstable; it may change at any time, in breaking or non-breaking ways, | ||
| /// including in SemVer minor releases. This requires the `unstable` Cargo feature. | ||
| /// </div> | ||
| #[cfg(feature = "unstable")] |
There was a problem hiding this comment.
thought: yay, hidden replaced by a feature, always good to see
| #[doc = icu_provider::gen_buffer_unstable_docs!(UNSTABLE, Self::new)] | ||
| /// | ||
| /// <div class="stab unstable"> | ||
| /// 🚧 This method is considered unstable; it may change at any time, in breaking or non-breaking ways, | ||
| /// including in SemVer minor releases. This requires the `unstable` Cargo feature. | ||
| /// </div> | ||
| #[cfg_attr(not(feature = "unstable"), doc(hidden))] |
There was a problem hiding this comment.
Suggestion, here and elsewhere: should the warning go inside gen_buffer_unstable_docs?
There was a problem hiding this comment.
eventually, but only once I added the feature to all crates
There was a problem hiding this comment.
Can you make it #[doc = icu_provider::gen_buffer_unstable_docs!(UNSTABLE_WITH_FEATURE, Self::new)] to reduce churn later
| pub trait IntoAnyCalendar: Calendar + Sized { | ||
| /// Convert this calendar into an [`AnyCalendar`], moving it | ||
| /// | ||
| /// You should not need to call this method directly | ||
| #[cfg_attr(not(feature = "unstable"), doc(hidden))] |
There was a problem hiding this comment.
Hmm, I hadn't considered these functions to be unstable. I expect client code to be able to use the functions on the Calendar and IntoAnyCalendar traits in a stable way.
| /// Internal scaffolding types | ||
| #[cfg_attr(not(feature = "unstable"), doc(hidden))] | ||
| pub mod scaffold { |
There was a problem hiding this comment.
Thought: Types in scaffold are not unstable in the same way as types in provider; it is still safe to make bounds based on them. Discuss?
In this particular instance, UnstableSealed is good to tag in this way, but maybe not the whole module?
There was a problem hiding this comment.
Agreed.
- UnstableSealed should be feature-gated
- The traits themselves should be always-public: people will need them for bounds
- The trait methods should be feature-hidden
components/calendar/src/calendar.rs
Outdated
| @@ -26,14 +26,17 @@ use core::fmt; | |||
| /// </div> | |||
| pub trait Calendar: crate::cal::scaffold::UnstableSealed { | |||
| /// The internal type used to represent dates | |||
| #[cfg_attr(not(feature = "unstable"), doc(hidden))] | |||
There was a problem hiding this comment.
I am not currently convinced that the types and methods on Date should be marked unstable.
Added an unstable feature to
icu_calendar.Rendered: https://icu4x-pr-artifacts.storage.googleapis.com/gha/974b65fcb6022766f3287a17f2ee8b718e6650b5/rustdoc/icu_calendar/index.htmlOne thing to point out is the constructors: https://icu4x-pr-artifacts.storage.googleapis.com/gha/974b65fcb6022766f3287a17f2ee8b718e6650b5/rustdoc/icu_calendar/cal/enum.AnyCalendar.html#implementationsSemver CI expectedly breaks, as APIs that were previously only documented to be unstable are now behind the unstable feature.