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

Pre-PR: Please review year/month Duration, date iteration #184

Closed
kosta opened this issue Sep 4, 2017 · 8 comments
Closed

Pre-PR: Please review year/month Duration, date iteration #184

kosta opened this issue Sep 4, 2017 · 8 comments

Comments

@kosta
Copy link

kosta commented Sep 4, 2017

Hi!

I wrote a small library that provides the following on top of chrono:

https://github.com/kosta/date-iterator

This has some rough edges as some stuff can only be implemented in chrono itself, e.g. I cannot do impl<Tz: TimeZone> Add<CalendarDuration> for DateTime<Tz> outside of chrono.

Please let me know whether you like this general direction and would consider merging such functionality in a PR. In that case I would prepare a branch for merging. I'm also happy to discuss & adjust any design choices.

Thanks in advance for your consideration! :)

Cheers,
Kosta

@kosta
Copy link
Author

kosta commented Sep 18, 2017

Did you get a chace to take a look? :) Maybe it makes more sense to chat about this?

@quodlibetor
Copy link
Contributor

Thanks for this! I really want this kind of functionality in chrono, there are some open questions about what the best design should look like. In particular we are thinking of adding a YearMonth enum to get rid of raw ints for months and to enable some nicer APIs.

I haven't looked too deeply at your code, but it appears to be more "duration-focused" than "instant-focused", if that makes sense? I am certain that we would need something like your CalendarDuration to be able to implement all the iterators that we want, but I'm not sure if our desire to change the chrono data model influences your opinion of what a good duration and iterator API would look like?

I'll take a closer look at your code over the weekend, and yes I will be happy to chat about it! I'm very excited that you're interested in this!

@kosta
Copy link
Author

kosta commented Sep 27, 2017

Thank you for taking a look! Yes, this is totally duration-focused.

If you're interested, I can merge this into a chrono fork, just to see what it would look like. This would allow to see how the API can be improved by this being part of chrono.

Let's chat next week :)

@quodlibetor
Copy link
Contributor

If you don't mind the possibility that we'll refactor heavily then I think that yes, merging into a chrono fork makes sense. Feel free to try and improve the chrono data model to make it easier to implement! I really want some direct experience of what we can do to make the entire project easier to work with.

@kosta
Copy link
Author

kosta commented Sep 29, 2017

Cool, I hope I can whip something up early next week!

@kosta
Copy link
Author

kosta commented Aug 24, 2018

I tried a more logical approach in the date-op branch. It's not 100% done yet but I think this makes the most sense:

https://github.com/kosta/chrono/tree/date-op
(All the changes from the current date-iterator branch are in this commit: kosta@af52b1b)

It takes care of both issues you opened kosta/date-iterator#1 and kosta/date-iterator#2

The date-iterators stayed basically the same, but CalendarDuration is replaced by the DateOp interface.

It needs more tests, docs and a special-case handling for MonthDuration(..., Skip) in the date-iterators plus maybe a few more ops (end of day/month/year; maybe event setDay/month/year etc.).

But apart from that, I think this should work out. What do you think?

CC'ing mayurdzk for their interest in this functionality

(Edit: If it's looks kinda-ok, I will merge it into this branch to update this PR)

@peckpeck
Copy link

peckpeck commented May 3, 2021

why only month and years, you also should add week and days, since they also have varying duration because of dst.

This would interest me

@pitdicker
Copy link
Collaborator

We now have a dedicated issue on adding a CalendarDuration type with these features: #1282.

@pitdicker pitdicker closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2023
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

4 participants