Repository: time-rs/time (language: rust)

Issue:
Add `const fn new() -> Self` to `time::format_description` types?
I know you have the `#[doc(hidden)] const fn default() -> Self` so that things like `time::format_description::modifier::Year::default()` work, and I see the comment that it's a hack until `Default::default()` by itself becomes legal in a const context. But since there are no plans for doing that (RFC 2632 was closed in 2021-07), it would be nice to have a non-`#[doc(hidden)]` of doing that, like a documented `const fn new() -> Self`.

I was actually going to make a PR to add a `const fn new() -> Self` to the macro output when I discovered this existing `#[doc(hidden)]` function. I imagine there must be other users who came in with the prior knowledge of `Default::default()` not working, didn't see any `const` constructors in the documentation, and assumed it just isn't possible to construct these values in a const context.

(Also, while it's true that an `fn new() -> Self` without parameters is redundant with a `Default` impl in general, it's still better for discoverability to have an `fn new`.)


Discussion / hints:
While that specific RFC was closed, it's nearly certain that it will happen in some form eventually; the standard library even has some `impl const Trait` internally (unstable, of course). The question is what form it will take.

I'd like to avoid having `const fn new() -> Self` because it's not clear what the stored values would be.

If your concern is being able to construct modifiers in a `const` context, you can still use `T::default()`. The comment could be reworded — `T::default()` being const is something I'm willing to guarantee. The only thing that's not guaranteed is that it'll always be its own method. If a user has `#![no_prelude]` enabled, it _would_ make a difference. As far as I know that's the only situation where it would actually matter.

Basically, the only thing I want to be able to do is remove `T::default()` as an inherent method once `impl const Default` is able to be done. The method itself you may rely on.
>The method itself you may rely on.

Okay, then would you be okay with:

```diff,rust
 macro_rules! impl_const_default {
     ($($type:ty => $default:expr;)*) => {$(
         impl $type {
-            /// A hack to work around the lack of const traits. Once const traits are stabilized,
-            /// this method will be removed in favor of `impl const Default`. As the `Default` trait
-            /// is in the prelude, this should not cause any resolution failures. Regardless, this
-            /// method is explicitly **not** part of the stable API of the time crate. It exists
-            /// solely for use in macros.
-            #[doc(hidden)]
+            /// Creates an instance of this modifier with default values.
+            ///
+            /// This function exists since `Default::default()` cannot be used in a const context,
+            /// and will be removed once that becomes possible. As the `Default` trait is in the prelude,
+            /// removing this function in the future should not cause any resolution failures.
             pub const fn default() -> Self {
                 $default
             }
         }
```

?

It would also be good to document what exactly the defaults are for each type, especially for the enums and for the structs with non-default defaults like bool fields that default to `true`.
That generally looks good, but it should definitely make a mention of [`#![no_implicit_prelude]`](https://doc.rust-lang.org/reference/names/preludes.html#the-no_implicit_prelude-attribute) to cover the edge case. Notably, it will **not** be considered breaking to remove the method.

Fix patch:
diff --git a/src/format_description/modifier.rs b/src/format_description/modifier.rs
index 7605b2a69..8cad3c3ee 100644
--- a/src/format_description/modifier.rs
+++ b/src/format_description/modifier.rs
@@ -119,7 +119,7 @@ pub struct Year {
     pub padding: Padding,
     /// What kind of representation should be used?
     pub repr: YearRepr,
-    /// Whether the value based on the ISO week number.
+    /// Whether the value is based on the ISO week number or the Gregorian calendar.
     pub iso_week_based: bool,
     /// Whether the `+` sign is present when a positive year contains fewer than five digits.
     pub sign_is_mandatory: bool,
@@ -242,23 +242,44 @@ pub enum Padding {
     None,
 }
 
+macro_rules! doc_hack {
+    (pub $(#[$attr:meta])*; $($x:tt)*) => {
+        $(#[$attr])*
+        ///
+        /// This function exists since [`Default::default()`] cannot be used in a `const` context.
+        /// It may be removed once that becomes possible. As the [`Default`] trait is in the
+        /// prelude, removing this function in the future will not cause any resolution failures for
+        /// the overwhelming majority of users; only users who use `#![no_implicit_prelude]` will be
+        /// affected. As such it will not be considered a breaking change.
+        $($x)*
+    };
+
+    ($(#[$attr:meta])*; $($x:tt)*) => {
+        $(#[$attr])*
+        ///
+        /// A hack to work around the lack of const trait impls. This method is explicitly **not**
+        /// part of the stable API of the time crate and may be removed at any point.
+        #[doc(hidden)]
+        $($x)*
+    };
+}
+
 /// Implement `Default` for the given type. This also generates an inherent implementation of a
 /// `default` method that is `const fn`, permitting the default value to be used in const contexts.
 // Every modifier should use this macro rather than a derived `Default`.
 macro_rules! impl_const_default {
-    ($($type:ty => $default:expr;)*) => {$(
+    ($($(#[$doc:meta])* $(@$pub:ident)? $type:ty => $default:expr;)*) => {$(
         impl $type {
-            /// A hack to work around the lack of const traits. Once const traits are stabilized,
-            /// this method will be removed in favor of `impl const Default`. As the `Default` trait
-            /// is in the prelude, this should not cause any resolution failures. Regardless, this
-            /// method is explicitly **not** part of the stable API of the time crate. It exists
-            /// solely for use in macros.
-            #[doc(hidden)]
-            pub const fn default() -> Self {
-                $default
+            doc_hack! {
+                $($pub)?
+                $(#[$doc])*;
+                pub const fn default() -> Self {
+                    $default
+                }
             }
         }
 
+        $(#[$doc])*
         impl Default for $type {
             fn default() -> Self {
                 $default
@@ -268,50 +289,83 @@ macro_rules! impl_const_default {
 }
 
 impl_const_default! {
-    Day => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero).
+    @pub Day => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value uses the
+    /// [`Numerical`](Self::Numerical) representation.
     MonthRepr => Self::Numerical;
-    Month => Self {
+    /// Creates an instance of this type that indicates the value uses the
+    /// [`Numerical`](MonthRepr::Numerical) representation, is [padded with zeroes](Padding::Zero),
+    /// and is case-sensitive when parsing.
+    @pub Month => Self {
         padding: Padding::default(),
         repr: MonthRepr::default(),
         case_sensitive: true,
     };
-    Ordinal => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero).
+    @pub Ordinal => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value uses the [`Long`](Self::Long) representation.
     WeekdayRepr => Self::Long;
-    Weekday => Self {
+    /// Creates a modifier that indicates the value uses the [`Long`](WeekdayRepr::Long)
+    /// representation and is case-sensitive when parsing. If the representation is changed to a
+    /// numerical one, the instance defaults to one-based indexing.
+    @pub Weekday => Self {
         repr: WeekdayRepr::default(),
         one_indexed: true,
         case_sensitive: true,
     };
-    WeekNumberRepr => Self::Iso;
-    WeekNumber => Self {
+    /// Creates a modifier that indicates that the value uses the [`Iso`](Self::Iso) representation.
+    @pub WeekNumberRepr => Self::Iso;
+    /// Creates a modifier that indicates that the value is [padded with zeroes](Padding::Zero)
+            /// and uses the [`Iso`](WeekNumberRepr::Iso) representation.
+    @pub WeekNumber => Self {
         padding: Padding::default(),
         repr: WeekNumberRepr::default(),
     };
+    /// Creates a modifier that indicates the value uses the [`Full`](Self::Full) representation.
     YearRepr => Self::Full;
-    Year => Self {
+    /// Creates a modifier that indicates the value uses the [`Full`](YearRepr::Full)
+    /// representation, is [padded with zeroes](Padding::Zero), uses the Gregorian calendar as its
+    /// base, and only includes the year's sign if necessary.
+    @pub Year => Self {
         padding: Padding::default(),
         repr: YearRepr::default(),
         iso_week_based: false,
         sign_is_mandatory: false,
     };
-    Hour => Self {
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero) and
+    /// has the 24-hour representation.
+    @pub Hour => Self {
         padding: Padding::default(),
         is_12_hour_clock: false,
     };
-    Minute => Self { padding: Padding::default() };
-    Period => Self {
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero).
+    @pub Minute => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value uses the upper-case representation and is
+    /// case-sensitive when parsing.
+    @pub Period => Self {
         is_uppercase: true,
         case_sensitive: true,
     };
-    Second => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero).
+    @pub Second => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the stringified value contains [one or more
+    /// digits](Self::OneOrMore).
     SubsecondDigits => Self::OneOrMore;
-    Subsecond => Self { digits: SubsecondDigits::default() };
-    OffsetHour => Self {
+    /// Creates a modifier that indicates the stringified value contains [one or more
+    /// digits](SubsecondDigits::OneOrMore).
+    @pub Subsecond => Self { digits: SubsecondDigits::default() };
+    /// Creates a modifier that indicates the value uses the `+` sign for all positive values
+    /// and is [padded with zeroes](Padding::Zero).
+    @pub OffsetHour => Self {
         sign_is_mandatory: true,
         padding: Padding::default(),
     };
-    OffsetMinute => Self { padding: Padding::default() };
-    OffsetSecond => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero).
+    @pub OffsetMinute => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value is [padded with zeroes](Padding::Zero).
+    @pub OffsetSecond => Self { padding: Padding::default() };
+    /// Creates a modifier that indicates the value is [padded with zeroes](Self::Zero).
     Padding => Self::Zero;
 }
 


Test patch: