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

Iterators for Ranges of Dates with day steps #5875

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Nikita-str
Copy link

The issue: #5448

The PR implements iterators with day steps for Date ranges by next pipeline:
RataDie -> Iso::iso_from_fixed -> Date::new_from_iso

The PR is only partial solution for the issue because:

  • There no month/year-step iterator: what to do when there no such month in next year or such day in next month? Round up to a bound?
  • You can make iterations faster on small intervals of days for ArithmeticDate (at least if day step is less or equal than min possible days in a month of calendar)

@Nikita-str Nikita-str requested review from Manishearth, sffc and a team as code owners November 29, 2024 05:02
@sffc
Copy link
Member

sffc commented Dec 6, 2024

Thanks for your interest!

The linked issue is about date range formatting, not necessarily date increment calculations. Now, adding durations to dates is something we want, and which we have partially implemented.

I would support a PR adding Date::next_date and Date::prev_date fns. Not sure about the iterator because the use cases seem fairly limited for the amount of extra code it adds.

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the iteration API; @Manishearth wdyt?

@Manishearth
Copy link
Member

I don't want to add an entirely new API for this. A Step trait impl would be nice so that people can do for date in <start>..<end>, and that trait can be very easily implemented using the offset() function.

I do not see a large benefit of a dedicated API that lets you control the step size, especially since it only supports step sizes measured in days and no other unit.

@Nikita-str
Copy link
Author

The linked issue is about date range formatting

lol

I would support a PR adding Date::next_date and Date::prev_date fns.

understood

A Step trait impl would be nice so that people can do for date in <start>..<end>

Hmm... but Step trait is unstable, or do you mean to reduce functionality of the Iters to single day step?

@Manishearth
Copy link
Member

Oh, if it's unstable we should just not do this until it stabilizes.

@Nikita-str
Copy link
Author

So, close the PR then?

@sffc
Copy link
Member

sffc commented Dec 9, 2024

If a dedicated pub fn next_date(&self) has characteristics that make it smaller/faster than pub fn add(&self, DateDuration), then we could consider adding next_date.

@sffc
Copy link
Member

sffc commented Dec 9, 2024

The linked issue is about date range formatting

lol

Sorry about the misleading issue!

In general, if you want to make contributions, consider issues labeled good first issue, which should generally have more detail on what the goals are.

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

Successfully merging this pull request may close these issues.

3 participants