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

Feature request: please consider implementing new() constructor and Default trait #293

Closed
lucatrv opened this issue Dec 12, 2018 · 9 comments

Comments

@lucatrv
Copy link

lucatrv commented Dec 12, 2018

I know that this has been discussed before, but please consider what suggested in the Rust API guidelines:

https://rust-lang-nursery.github.io/api-guidelines/interoperability.html

Note that it is common and expected for types to implement both Default and an empty new constructor. new is the constructor convention in Rust, and users expect it to exist, so if it is reasonable for the basic constructor to take no arguments, then it should, even if it is functionally identical to default.

https://rust-lang-nursery.github.io/api-guidelines/predictability.html

Note that it is common and expected for types to implement both Default and a new constructor. For types that have both, they should have the same behavior. Either one may be implemented in terms of the other.

For a time type, I think its natural default value should be 0. For a date type, IMHO day 1 of month 1 of year 0 is a reasonable default value as well. Being able to derive the Default trait when defining a custom type which includes a chrono type would of course be very convenient.

@bspeice
Copy link

bspeice commented Jan 20, 2019

The point being made in the issue you linked to was that there isn't a good sense of what "default" is. Having a default of "the current naive time" for NaiveDateTime or "the current tz-aware local time" for DateTime are both sensible options in addition to what you mentioned.

When deriving Default, I think it makes more sense to wrap the DateTime<> in an Option. But can you explain a bit more on what the use case is?

@lucatrv
Copy link
Author

lucatrv commented Jan 25, 2019

Sure, I suggest to implement the Default trait so that one could write:

#[derive(Debug, Default)]
struct MyStruct {
    name: String,
    content: String,
    date: NaiveDate,
}
impl MyStruct {
    fn new() -> Self {
        Self::default()
    }
}

@quodlibetor
Copy link
Contributor

The thing that prevents us from implementing default is that there is no "obvious" default. Either one of several possible values of Zero or "now" both make sense.

You can either implement Default manually or do something like:

struct DefaultNow(pub NaiveDate);
impl Default for DefaultNow {
    fn default() -> DefaultNow {
        DefaultNow(Utc.today().naive_utc())
    }
}

Which will allow you to derive Default on your structs that contain a DefaultNow.

@rbtcollins
Copy link

Three small observations if I may;

since using Default requires opting in from higher level code, there seems to be little chance of negative effects if the wrong arm of the choice (0 vs now()) were to be made.

Secondly, if an arbitrary choice were made, for the folk for whom it is not suitable, a wrapper type can still be used to get the desired customisation.

Lastly, for test code having deterministic defaults is highly desirable, so I would very much encourage a zero-valued default implementation of default, leaving now() as an opt-in facility for cases where it is appropriate.

@sanmai-NL
Copy link

Exactly right @rbtcollins. For some reason, many Rust crate maintainers I encountered are apprehensive because they're unsure which default to pick. That‘s beside the point though. Why would it matter which one among a set of reasonable options you pick?

@brianbruggeman
Copy link

brianbruggeman commented Feb 25, 2021

Sometimes moving forward "wrongly" is better than not moving forward at all. For very large structures, having to implement Default can become cumbersome and reminds me of XML and Java boilerplate. I would argue for picking one default and then letting it be.

That said, an addendum to @quodlibetor 's solution for structures that are more complex (e.g. nested structures). One only needs to include ..Default::default() if Default is derivable for all of the fields that actually have a defined default. But notably, I've now added 10 extra lines for my 6 line struct. Still smells, imo.

struct Complex {
     ...
     date_created: NaiveDate,
     date_modified: NaiveDate,
     ...
}

impl Default for Complex {
    fn default() -> Self {
        let now = Utc.today().naive_utc();
        Self {
            date_created: now,
            date_modified: now,
            ..Default::default()
        }
    }
}

@vdods
Copy link

vdods commented Mar 30, 2022

I'll add my support for implementing Default for DateTime<Tz>, defining it to be the beginning of the Unix epoch.

  • Unix time is a de-facto canonical time format broadly used in software (e.g. JWT's NumericDate, with DateTime<Utc> being the chrono analog of it).
  • Assuming that we accept use of Unix time, because the zero value is mathematically canonical (in the sense that it is a value guaranteed to exist), it is really the only sensible choice as the default value.
  • This provides a deterministic value for Default, whereas "now" would not.
  • Why Unix epoch zero instead of 0000-01-01? I think it's important that the default value we decide upon have valid and exact (and round-trippable) conversions to other common date types, such as f64 (as in the case of JWT's NumericDate) or an i32 valued "number of seconds from beginning of Unix epoch". By definition, these support Unix time 0. But it seems likely that representing 0000-01-01 in a f64 NumericDate would incur rounding error, while i32 can't represent it at all (it's bounded by ~68 years on either side of 1970).

Regarding NaiveDateTime, by definition it has no notion of timezone, and so its relation to UTC is unknown. But I would suggest that it's important to define the default value for NaiveDateTime to be the "naive" beginning of the Unix epoch, i.e. 1970-01-01 00:00:00, so that the various definitions of of default agree relative to the UTC timezone. Using category theory terminology, we want the relevant diagram to commute. We want the following to be true:

  • DateTime::<Utc>::default().naive_utc() == NaiveDateTime::default()

@djc
Copy link
Contributor

djc commented Mar 30, 2022

I can see how the lack of a Default impl is painful from an ergonomic perspective due to how Default derives work, but I'm not going to add one. I agree with @quodlibetor's assessment that there is no single obvious (enough) default value. I'm going to close this. Unless you're going to add new arguments that have not actually been raised in this thread before, please don't comment or reopen this issue.

@djc djc closed this as completed Mar 30, 2022
@1Dragoon
Copy link

1Dragoon commented May 23, 2022

Perhaps not a default() or even a new() as their meanings would be ambiguous, but it would be somewhat useful to have a few DateTime constructors that can represent say unix epoch 32-bit zero and max, and 64-bit zero, max. The use case here is that I'm trying to coalesce an optional DateTime as unix timestamp zero, the idea being that if no datetime is specified in a json file, then it will coalesce as unix zero, effectively making a file date search filter have a meaning of "all time". I think there may be other use cases as well.

The alternative in some cases is to do something like

DateTime::parse_from_rfc2822("0000-01-01T00:00:00-00:00").unwrap()

Or in every case we run into the optional, we have to do

if let Some(t) {
   ...
} else {
  ...
}

Both of which feel a bit kludgy (the later contributing to the big nested ifs we have to deal with in the case of too many optionals)

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

No branches or pull requests

9 participants