-
Notifications
You must be signed in to change notification settings - Fork 517
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
NaiveDateTime
to DateTime
helper method
#711
Conversation
49b5d02
to
6afb356
Compare
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
Given all this, once the upcoming The challenge here is to come up with an API that meets the following criteria:
Given the two options here of: The first has the advantage of using the After all this my conclusions are:
Please let me know your thoughts |
@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 |
Hi @botahamec - potentially we could also call it |
That'd be incorrect. It currently returns a |
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 |
16df3a2
to
9b1dc09
Compare
@esheppa I decided to go with |
/// Converts the `NaiveDateTime` into the timezone-aware `DateTime<Tz>` | ||
/// with the provided timezone, if possible. | ||
/// | ||
/// This is experimental and might be removed in the future. Feel free to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we calling this experimental again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was proposed in this comment. Apparently this API gets reinvented a lot. #711 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think labeling this as experimental isn't helpful because it's not clear what downstream users should do with that information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @botahamec |
fixes #687
I chose
and_timezone
as the method name. The issue proposes usingto_datetime_local
. I can see pros and cons with both, so let me know if we want to use a different name