Skip to content

support Duration in Date.range/3 #14172

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Jan 13, 2025
Merged

support Duration in Date.range/3 #14172

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
563fd96
support `Duration` in `Date.range/3`
tfiedlerdejanze Jan 10, 2025
c0b0dd4
update guard and argument error
tfiedlerdejanze Jan 10, 2025
c480baf
support keyword list durations
tfiedlerdejanze Jan 11, 2025
a31cead
consolidate t:Date.duration_unit_pair()
tfiedlerdejanze Jan 11, 2025
99c0fab
docs
tfiedlerdejanze Jan 11, 2025
8e0c543
redundant call of __duration__!/1
tfiedlerdejanze Jan 11, 2025
ab237e9
unused arg
tfiedlerdejanze Jan 11, 2025
228f1df
improved spec based on comments
tfiedlerdejanze Jan 13, 2025
104d7b9
omit additional ArgumentError for untyped args
tfiedlerdejanze Jan 13, 2025
c9279d1
unused args
tfiedlerdejanze Jan 13, 2025
66de751
keep ArgumentError clause for invalid step
tfiedlerdejanze Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions lib/elixir/lib/calendar/date.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,10 @@ defmodule Date do

"""
@doc since: "1.5.0"
@spec range(first :: Calendar.date(), last_or_duration :: Calendar.date() | Duration.t() | [duration_unit_pair]) ::
@spec range(
first :: Calendar.date(),
last_or_duration :: Calendar.date() | Duration.t() | [duration_unit_pair]
) ::
Date.Range.t()
def range(%{calendar: calendar} = first, %{calendar: calendar} = last) do
{first_days, _} = to_iso_days(first)
Expand Down Expand Up @@ -153,10 +156,6 @@ defmodule Date do
range(first, last)
end

def range(%{calendar: _, year: _, month: _, day: _}, _duration) do
raise ArgumentError, "expected a date or duration as second argument"
end

@doc """
Returns a range of dates with a step.

Expand Down Expand Up @@ -208,12 +207,6 @@ defmodule Date do
range(first, last, step)
end

def range(%{calendar: _, year: _, month: _, day: _} = first, duration, step) do
raise ArgumentError,
"expected a date or duration as second argument and the step must be a " <>
"non-zero integer, got: #{inspect(first)}, #{inspect(duration)}, #{step}"
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was removed following this comment from @whatyouhide, but a zero step won't be caught by the typesystem so I think we should probably still raise a proper ArgumentError in this case, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

yes the zero step yep, good callout

Copy link
Contributor Author

@tfiedlerdejanze tfiedlerdejanze Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the basic types an invalid step should be caught by the type system (pos_integer, neg_integer), wouldn't it?

However, as it stands it's not consistent as Date.range(d1, d2, 0) raises an ArgumentError and Date.range(d1, duration, 0) raises a FunctionClauseError.

I'll re-add the clauses with the adjusted ArgumentError message to range/2 and range/3. Actually limiting it to range/3 feels right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also, how about not matching or guarding durations at all (as we also don't do it in shift/2)? so we'd just have:

def range(%{calendar: calendar} = first, %{calendar: calendar} = last)
def range(%{calendar: _, year: _, month: _, day: _}, %{calendar: _, year: _, month: _, day: _})
def range(%{calendar: _} = first, duration)

and

def range(%{calendar: calendar} = first, %{calendar: calendar} = last, step) when is_integer(step) and step != 0
def range(%{calendar: _, year: _, month: _, day: _}, %{calendar: _, year: _, month: _, day: _}, step)
def range(%{calendar: _} = first, duration) when is_integer(step) and step != 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the basic types an invalid step should be caught by the type system (pos_integer, neg_integer), wouldn't it?

These are the "old types" (typespecs), for now AFAIK there is no plan to implement these in the new set-theoretic types.

Copy link
Contributor

@sabiwara sabiwara Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps we should clarify this distinction in the docs, I can see how having two type systems can be confusing even if it is just a transition phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh actually wasn't aware of that distinction! thanks!


defp range(first, first_days, last, last_days, calendar, step) do
%Date.Range{
first: %Date{calendar: calendar, year: first.year, month: first.month, day: first.day},
Expand Down
8 changes: 0 additions & 8 deletions lib/elixir/test/elixir/calendar/date_range_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,6 @@ defmodule Date.RangeTest do
end
end

test "second argument is validated" do
first = ~D[2000-01-01]

assert_raise ArgumentError, "expected a date or duration as second argument", fn ->
Date.range(first, 123)
end
end

test "accepts equal but non Calendar.ISO calendars" do
first = Calendar.Holocene.date(12000, 1, 1)
last = Calendar.Holocene.date(12001, 1, 1)
Expand Down