NaiveDateTime to DateTime helper method

[botahamec]

fixes #687

I chose and_timezone as the method name. The issue proposes using to_datetime_local. I can see pros and cons with both, so let me know if we want to use a different name

[esheppa]

Thanks @botahamec. This is an interesting function however there is some history surrounding this, specifically here

That said, there may be an argument for this way of generating a DateTime<Tz> using ymd/hms style functions, as the current way (Utc.ymd().hms()) requires the Date which is also listed on the same page:

Date<Tz> doesn't actually make sense.

Given all this, once the upcoming 0.4.20 release is out, there will likely be some scope for larger changes in the API for a 0.5

The challenge here is to come up with an API that meets the following criteria:

• Encourages use of DateTime with TimeZone to avoid issues that come from using the NaiveDateTime style
• Relatively ergonomic
• One way to do things

Given the two options here of:
1.tz.from_local_datetime(ndt)
2. ntd.and_timezone(tz)

The first has the advantage of using the TimeZone trait, however I'm quite persuaded by the reasoning about the IDE friendliness of having inherent methods (in 0.5 we could also consider applying similar logic with Datelike and Timelike).

After all this my conclusions are:

• Merge this in as it would be interesting to get feedback on whether people like using this API (perhaps we could document that it is experimental and might be removed/replaced in the future?)
• Potentially rename the function - it could be nice to include the word local, for example local_with_timezone

Please let me know your thoughts

[botahamec]

@esheppa I like those thoughts. If the idea is to see what people think of the API, we could add something in the documentation to say "let us know what you think of this using [currently unknown method]"

I think it makes sense to include "local" in the name, but I kinda struggled with figuring out the best name for it. After thinking about it for a bit, I think local_with_timezone makes sense, but it still seems a bit unintuitive. But now I'm wondering if that's just a problem that comes from trying to explain "this assumes that the given NativeDateTime is local to the given time zone" and maybe that's just supposed to be a given. At the same time, we're supposed to be getting feedback anyway, so maybe that's ok. I'll go with whatever.

[esheppa]

Hi @botahamec - potentially we could also call it assert_timezone - this is useful as this function has to either be potentially panic if returning a DateTime<Tz> or instead return LocalResult<DateTime<Tz>>. Given it currently returns the former, the assert in the name could suffice for both the 'from_local" part and the "can panic" part.

[botahamec]

this function has to either be potentially panic if returning a DateTime<Tz> or instead return LocalResult<DateTime<Tz>>. Given it currently returns the former, the assert in the name could suffice for both the 'from_local" part and the "can panic" part.

That'd be incorrect. It currently returns a LocalResult.

[esheppa]

My apologies @botahamec, I should have re-checked the code rather than relying on memory - happy to go with either of the options suggested, or a better one you can think of, if you update the name (or leave it as-is, if you think that it is the best option), and add the documentation about stability/use then this should be good to merge

[botahamec]

@esheppa I decided to go with and_local_timezone. I think this best matches the naming convention of the rest of the library. with is typically used to replace a part of the structure with something else, but and is used to change the type. I could go either way on whether it should be and_local_timezone or local_and_timezone.

[djc]

Why are we calling this experimental again?

[botahamec]

It was proposed in this comment. Apparently this API gets reinvented a lot. #711 (comment)

[djc]

I don't think labeling this as experimental isn't helpful because it's not clear what downstream users should do with that information.

[esheppa]

This is a really good point - happy to set up a PR to remove this line if you think that makes the most sense.

I'd noticed this elsewhere in the codebase, so figured there was some precedent for mentioning something as experimental/unstable - albeit this actually shows up in the readme so has a higher probability of actually getting feedback.

Another way to improve this is I could try to co-ordinate somewhere to actually get the feedback and provide a link in the docs (this could be interesting for other changes as well) - potentially github discussions on this repo could be an option, or just issues?

[djc]

A Cargo feature is useful there because it includes extra dependencies. Also IMO "unstable" (as opposed to "experimental") has a more or less well-known meaning, in that semver-stable updates can break that particular API. Including that in the default features is probably not great, since it's too easy to miss the documentation. So I think the precedent from unstable-locales isn't all that applicable here.

We could link to an issue or enable GitHub discussions, but it feels like overkill to me in this case -- it seems to me that this API is a decent addition that "paves a cowpath" in a manner consistent with other existing API.

In short, yes, IMO we could just remove the comment about this being experimental.

[esheppa]

Thanks @djc - that makes sense, I've submitted #715 to rectify this.

[esheppa]

Thanks @botahamec