support `Duration` in `Date.range/3` #14172

tfiedlerdejanze · 2025-01-10T20:48:20Z

No description provided.

josevalim · 2025-01-10T22:36:46Z

lib/elixir/lib/calendar/date.ex

@@ -119,6 +124,11 @@ defmodule Date do
    range(first, first_days, last, last_days, calendar, step)
  end

+  def range(%{calendar: calendar} = first, %Duration{} = duration) do


shall we allow the keyword list version as well? :)

i found that range(~D[2020-01-01], [year: 1], 2) looks a bit unergonomic, but definitely nothing speaking against allowing it 👍

sabiwara · 2025-01-10T23:14:05Z

lib/elixir/lib/calendar/date.ex

@@ -149,6 +159,11 @@ defmodule Date do
    range(first, first_days, last, last_days, calendar, step)
  end

+  def range(%{calendar: calendar} = first, %Duration{} = duration, step) do


We should probably guard this too to check that step is a non-zero integer and update the ArgumentError clause & message?

lib/elixir/lib/calendar/date.ex

whatyouhide · 2025-01-13T08:56:35Z

lib/elixir/lib/calendar/date.ex

@@ -84,6 +87,13 @@ defmodule Date do
      iex> Date.range(~D[1999-01-01], ~D[2000-01-01])
      Date.range(~D[1999-01-01], ~D[2000-01-01])

+  A range may also be built from a `Date` and a `Duration`:


Suggested change

A range may also be built from a `Date` and a `Duration`:

A range may also be built from a `Date` and a `Duration`

(also expressed as a keyword list of `t:duration_unit_pair/0`):

?

lib/elixir/lib/calendar/date.ex

whatyouhide · 2025-01-13T09:00:04Z

lib/elixir/lib/calendar/date.ex

@@ -100,7 +110,8 @@ defmodule Date do

  """
  @doc since: "1.5.0"
-  @spec range(Calendar.date(), Calendar.date()) :: Date.Range.t()
+  @spec range(Calendar.date(), Calendar.date() | Duration.t() | [duration_unit_pair]) ::


I think this would improve docs (now they would still show range(first, last):

Suggested change

@spec range(Calendar.date(), Calendar.date() | Duration.t() | [duration_unit_pair]) ::

@spec range(start :: Calendar.date(), end_or_duration :: Calendar.date() | Duration.t() | [duration_unit_pair]) ::

while I like start/end, should we stick to first/last so it's:

@spec range(first :: Calendar.date(), last_or_duration :: Calendar.date() | Duration.t() | [duration_unit_pair]) ::

last_or_duration is ok but the "duration" part doesn't work as well with first IMO, compared to start + duration/start + end. Either way not a big deal 🙃

whatyouhide · 2025-01-13T09:00:53Z

lib/elixir/lib/calendar/date.ex

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


@josevalim do we usually do this, or just let this fall through and raise a FunctionClauseError? I think this kind of thing would be caught by types anyway

whatyouhide · 2025-01-13T09:01:01Z

lib/elixir/lib/calendar/date.ex

+          Calendar.date(),
+          Calendar.date() | Duration.t() | [duration_unit_pair],


Same naming issue here

sabiwara · 2025-01-13T11:43:15Z

lib/elixir/lib/calendar/date.ex

-  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


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?

yes the zero step yep, good callout

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.

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

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.

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.

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

josevalim · 2025-01-13T18:39:45Z

💚 💙 💜 💛 ❤️

There is some ambiguity if the duration represents the end date or the step, so it is best to be explicit about it. This reverts commit 8deaaf4.

tfiedlerdejanze force-pushed the date-range-with-durations branch from 72659ab to 7650afb Compare January 10, 2025 20:52

tfiedlerdejanze changed the title ~~support Duration in Date.range/3~~ support Duration in Date.range/3 Jan 10, 2025

support Duration in Date.range/3

563fd96

tfiedlerdejanze force-pushed the date-range-with-durations branch from 7650afb to 563fd96 Compare January 10, 2025 21:01

josevalim reviewed Jan 10, 2025

View reviewed changes

sabiwara reviewed Jan 10, 2025

View reviewed changes

tfiedlerdejanze added 6 commits January 11, 2025 00:47

update guard and argument error

c0b0dd4

support keyword list durations

c480baf

consolidate t:Date.duration_unit_pair()

a31cead

docs

99c0fab

redundant call of __duration__!/1

8e0c543

unused arg

ab237e9

josevalim approved these changes Jan 11, 2025

View reviewed changes

whatyouhide reviewed Jan 13, 2025

View reviewed changes

tfiedlerdejanze added 3 commits January 13, 2025 10:13

improved spec based on comments

228f1df

omit additional ArgumentError for untyped args

104d7b9

unused args

c9279d1

sabiwara reviewed Jan 13, 2025

View reviewed changes

keep ArgumentError clause for invalid step

66de751

wojtekmach approved these changes Jan 13, 2025

View reviewed changes

josevalim merged commit 8deaaf4 into elixir-lang:main Jan 13, 2025
9 checks passed

sabiwara mentioned this pull request Jan 15, 2025

Mention the new type system in typespecs doc #14188

Merged

josevalim added a commit that referenced this pull request Jan 20, 2025

Revert "Support Duration in Date.range/3 (#14172)"

b15de1c

There is some ambiguity if the duration represents the end date or the step, so it is best to be explicit about it. This reverts commit 8deaaf4.

sabiwara added a commit to sabiwara/elixir that referenced this pull request Jan 20, 2025

Remove changelog entry after reverting elixir-lang#14172

f44d60b

sabiwara added a commit that referenced this pull request Jan 20, 2025

Remove changelog entry after reverting #14172 (#14202)

a42f2e8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support `Duration` in `Date.range/3` #14172

support `Duration` in `Date.range/3` #14172

tfiedlerdejanze commented Jan 10, 2025

josevalim Jan 10, 2025

tfiedlerdejanze Jan 10, 2025

sabiwara Jan 10, 2025

whatyouhide Jan 13, 2025

whatyouhide Jan 13, 2025

tfiedlerdejanze Jan 13, 2025

tfiedlerdejanze Jan 13, 2025

whatyouhide Jan 13, 2025

whatyouhide Jan 13, 2025

whatyouhide Jan 13, 2025

sabiwara Jan 13, 2025

whatyouhide Jan 13, 2025

tfiedlerdejanze Jan 13, 2025 •

edited

Loading

tfiedlerdejanze Jan 13, 2025

sabiwara Jan 13, 2025

sabiwara Jan 13, 2025 •

edited

Loading

tfiedlerdejanze Jan 13, 2025

josevalim commented Jan 13, 2025

	A range may also be built from a `Date` and a `Duration`:
	A range may also be built from a `Date` and a `Duration`
	(also expressed as a keyword list of `t:duration_unit_pair/0`):

	@spec range(Calendar.date(), Calendar.date() \| Duration.t() \| [duration_unit_pair]) ::
	@spec range(start :: Calendar.date(), end_or_duration :: Calendar.date() \| Duration.t() \| [duration_unit_pair]) ::

		Calendar.date(),
		Calendar.date() \| Duration.t() \| [duration_unit_pair],

support Duration in Date.range/3 #14172

support Duration in Date.range/3 #14172

Conversation

tfiedlerdejanze commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfiedlerdejanze Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabiwara Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Jan 13, 2025

support `Duration` in `Date.range/3` #14172

support `Duration` in `Date.range/3` #14172

tfiedlerdejanze Jan 13, 2025 •

edited

Loading

sabiwara Jan 13, 2025 •

edited

Loading