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

Tweak NaiveDateTime.from_erl and Time.from_erl specs #13987

Merged

Conversation

elfenlaid
Copy link
Contributor

The changes align NaiveDateTime.from_erl and Time.from_erl specs with their corresponding NaiveDateTime.new and Time.new specs.

The new function specs accept a non-negative integer or a tuple of {seconds, precision} for microseconds.

Here's NaiveDateTime.new/8's spec:

@spec new(
Calendar.year(),
Calendar.month(),
Calendar.day(),
Calendar.hour(),
Calendar.minute(),
Calendar.second(),
Calendar.microsecond() | non_neg_integer,
Calendar.calendar()
) :: {:ok, t} | {:error, atom}

And NaiveDateTime.from_erl's spec + implementation:

@spec from_erl(:calendar.datetime(), Calendar.microsecond(), Calendar.calendar()) ::
{:ok, t} | {:error, atom}
def from_erl(tuple, microsecond \\ {0, 0}, calendar \\ Calendar.ISO)
def from_erl({{year, month, day}, {hour, minute, second}}, microsecond, calendar) do
with {:ok, iso_naive_dt} <- new(year, month, day, hour, minute, second, microsecond),
do: convert(iso_naive_dt, calendar)
end

Here's the Time.new/5 spec:

@spec new(
Calendar.hour(),
Calendar.minute(),
Calendar.second(),
Calendar.microsecond() | non_neg_integer,
Calendar.calendar()
) :: {:ok, t} | {:error, atom}

And the corresponding from_erl:

@spec from_erl(:calendar.time(), Calendar.microsecond(), Calendar.calendar()) ::
{:ok, t} | {:error, atom}
def from_erl(tuple, microsecond \\ {0, 0}, calendar \\ Calendar.ISO)
def from_erl({hour, minute, second}, microsecond, calendar) do
with {:ok, time} <- new(hour, minute, second, microsecond, Calendar.ISO),
do: convert(time, calendar)
end

In both cases, the corresponding new functions handle non_negative_integer microseconds. I also added tests to confirm it.


Please notice that both NaiveDateTime.t.microsecond and Time.t.microsecond are of Calendar.microsecond() type, and it's okay because new functions set microsecond: {microsecond, 6} with default precision if none is provided.

@@ -252,7 +252,7 @@ defmodule NaiveDateTime do
Calendar.hour(),
Calendar.minute(),
Calendar.second(),
Calendar.microsecond() | non_neg_integer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure here, but the rest of the file uses non_neg_integer() spec notation, so I tweaked the spec here.

@@ -317,7 +317,7 @@ defmodule NaiveDateTime do
Calendar.hour(),
Calendar.minute(),
Calendar.second(),
Calendar.microsecond() | non_neg_integer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure here, but the rest of the file uses non_neg_integer() spec notation, so I tweaked the spec here.

@@ -115,7 +115,7 @@ defmodule Time do
Calendar.hour(),
Calendar.minute(),
Calendar.second(),
Calendar.microsecond() | non_neg_integer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure here, but the rest of the file uses non_neg_integer() spec notation, so I tweaked the spec here.

@@ -355,13 +355,21 @@ defmodule Time do

## Examples

iex> Time.from_erl({23, 30, 15})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs were lacking the basic from_erl case, from_erl! has it, so I added one here too.

@josevalim josevalim merged commit 2c3a906 into elixir-lang:main Nov 12, 2024
12 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants