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

Allow getting the PrimitiveDateTime of a OffsetDateTime #458

Closed
wants to merge 1 commit into from
Closed

Allow getting the PrimitiveDateTime of a OffsetDateTime #458

wants to merge 1 commit into from

Conversation

GunnarMorrigan
Copy link
Contributor

@GunnarMorrigan GunnarMorrigan commented Apr 11, 2022

This is already possible through getting the date and time and making a primitive but would just be nicer to have easy API.

@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #458 (0edb241) into main (a957e12) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #458      +/-   ##
==========================================
- Coverage   99.74%   99.69%   -0.05%     
==========================================
  Files          62       62              
  Lines        6265     6268       +3     
==========================================
  Hits         6249     6249              
- Misses         16       19       +3     
Impacted Files Coverage Δ
src/offset_date_time.rs 99.25% <0.00%> (-0.75%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a957e12...0edb241. Read the comment docs.

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

This was previously declined in #369, #390, #395, and #454. It is highly recommended to search before opening an issue or PR.

@jhpratt jhpratt closed this Apr 11, 2022
@jhpratt jhpratt added C-duplicate Category: exact duplicate wontfix A-core Area: anything not otherwise covered labels Apr 11, 2022
@GunnarMorrigan
Copy link
Contributor Author

GunnarMorrigan commented Apr 11, 2022

This was previously declined in #369, #390, #395, and #454. It is highly recommended to search before opening an issue or PR.

Why would this not be allowed? Obviously many people would like to do this. It is only logical as for many instances you do not want to have any Offset at all. Just the pure UTC and this will enforce only UTC by type.

The usage of time in programming does not necessarily require an offset handling. In many calculations just the actual time passed is needed. If it is in Offset UTC or Offset +5 does not matter.

Besides that there is also no way that new() is as fast as copying the underlying data structure directly.

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

Please read the linked issues. I have repeatedly articulated my reasons for declining this request and will not do so again here.

@GunnarMorrigan
Copy link
Contributor Author

Please read the linked issues. I have repeatedly articulated my reasons for declining this request and will not do so again here.

I have read those comments. Fine that you wish that people be safe with Time and 'most' people need the offset.
That does not mean all people need the offset. This is just a lacking API.

The argument of internal representation changing is flawed due to the fact that PrimitiveDateTime is also a public struct and thus using date and time to create it will then likely also be broken due to this internal restructuring.

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

People want the utc_datetime field exposed largely because it's a field. That's terrible reasoning.

You want to discard information that is absolutely necessary. I have yet to see a use case where this API is actually needed. To give you an idea of how easy it is to get wrong, your comment is wrong.

This is already possible through getting the date and time and making a primitive

This is not what you're doing in the PR. Time is complicated, and you should never discard information you have about the offset. Doing so is extremely error prone.

Unless you truly have something new to add, I'm not going to keep responding. I've been over this before.

@GunnarMorrigan
Copy link
Contributor Author

GunnarMorrigan commented Apr 11, 2022

This is not what you're doing in the PR. Time is complicated, and you should never discard information you have about the offset. Doing so is extremely error prone.

Are you saying that creating an OffsetDateTime using OffsetDateTime::now_utc() then getting date and time and creating a PrimitiveDateTime is not the same as my PR?

Please enlighten me on why it is not the same, cause i would love to know...

Doing so is extremely error prone.

Yeah only when you actually need the offset. If you actually do not need the offset it is not. If you read my comment above time passing is not different in UTC or +5 offsets.

Anyways, you can do what you want but it is apparent that people would like this in the API and you are unwilling to provide such access based on flawed arguments.

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

If you need the time passed, you should use Duration::time_fn or Instant. Anything else is error-prone. Clocks aren't guaranteed to tick evenly, or even move forward in time.

If you call now_utc, it is the same. In any situation where the offset is not UTC the result would not be the same.

Stop calling my arguments "flawed". I have clearly articulated my reasons. I have far more experience with time handling and my goal is to reduce the amount of things you do wrong. Not implementing this request is part of doing that.

@GunnarMorrigan
Copy link
Contributor Author

If you call now_utc, it is the same. In any situation where the offset is not UTC the result would not be the same.

I am well aware. Thank you! I only want to do time arithmetic in the UTC timezone so keeping a timezone is useless.

Stop calling my arguments "flawed".

Well, sadly they just are partially flawed. You can not use Duration if the Duration is yet unknown. I can not use Instant as it does not provide the full functionality required to do complex calculations and your offset eats into my limited embedded memory.

If i had plenty to spare I would really be fine with your view and technically I truly understand it. But this just wastes compute cycles and memory.

Anyways, I do not think we will figure this out. Which tbh, is kinda sad as your crate is all around the best related to time.
Just think about it and someday you might change your view. No hard feelings so, have a good evening/day!

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

I only want to do time arithmetic in the UTC timezone so keeping a timezone is useless.

It's semantic information that is certainly not useless.

You can not use Duration if the Duration is yet unknown

I mentioned Duration::time_fn, which creates a Duration.

You mention "complex calculations" but do not elaborate. Without an actual use case, which I've already indicated I do not have, I have no reason to add an API like this. You need to explain why Instant isn't sufficient. Four bytes is very little, even on the smallest targets Rust supports.

@GunnarMorrigan
Copy link
Contributor Author

GunnarMorrigan commented Apr 11, 2022

Duration::time_fn is just times a function. I have to do physics related calculations on signals.

Instant is the most bare bones API that you can imagine. It does not even allow simple replacing of sub elements.

You mention "complex calculations" but do not elaborate

I can't it has to do with signal processing. I recognize working with time is hard that is the sole reason I want to use your crate. But the inability to go from Offset to Primitive just makes it all the more awkward.

If instants had a more elaborate API (similar to Primitive), I would use it. Is this something that you do feel ok with (allowing) extending?

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

You and I both know that you edited Duration::time_fn.

Within one minute of posting because I got the method name wrong. I never suggested using the entirety of Duration, nor do I appreciate the implication that I did. Edits are fully public, including the time they occurred.

simple replacing of sub elements.

What's that?

I can't it has to do with signal processing.

Then I'm afraid I can't help you in any way. The lack of a defined use case does not lead to things getting added. Declining to elaborate just means I still don't have a use case.

If instants had a more elaborate API (similar to Primitive), I would use it. Is this something that you do feel ok with (allowing) extending?

time::Instant exists to permit some nicer APIs with time::Duration. Nothing else. It is otherwise identical to std::time::Instant. It is fundamentally opaque. These properties are all documented.

@GunnarMorrigan
Copy link
Contributor Author

lets just agree that this wont work out. I don't see the point in arguing with you. You and I are both set in our ways and it seems that there is little room for an agreement.

The lack of a defined use case does not lead to things getting added. Declining to elaborate just means I still don't have a use case.

I never implied such a thing. I have accepted that it won't be added. If I was allowed to share more information I would.

What's that?

Replacing of seconds, nanoseconds, etc.

I would like to add that there is something to say for providing access to the primitive.
Calling such a function would need to yield a primitive that has not been adjusted for different offsets.
As that is the whole merit of time Primitives that they are not offset depended or free from offsets and are solely based on UTC.
With the calls to Date and Time the whole premise of pureness of the primitive is discarded.

Anyways, Jacob, if you are still not inclined to add the functionality I would like to thank you for your time and work on this crate. Best wishes :)

@jhpratt
Copy link
Member

jhpratt commented Apr 11, 2022

I'm not set in my ways, I just haven't seen a use case.

Instant doesn't have the concept of seconds and nanoseconds. It's an opaque value, even to me. Ultimately there is little I can do with the information I've been provided.

As that is the whole merit of time Primitives that they are not offset depended or free from offsets and are solely based on UTC.

This is the fundamental point. Primitive means there is no offset or an unknown offset, not that the offset is UTC.

@time-rs time-rs locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-core Area: anything not otherwise covered C-duplicate Category: exact duplicate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants