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

Support negative dates in Calendar #6638

Merged
merged 1 commit into from Feb 11, 2018

Conversation

Projects
None yet
4 participants
@lexmag
Member

lexmag commented Oct 7, 2017

Closes #6294.

@@ -332,11 +336,15 @@ defmodule Calendar.ISO do
defp sign(total) when total < 0, do: "-"
defp sign(_), do: "+"
defp zero_pad(val, count) do
defp zero_pad(val, count) when val >=0 do

This comment has been minimized.

@josevalim

josevalim Oct 10, 2017

Member

>=0

@@ -250,6 +250,12 @@ defmodule Date do
end
end
def from_iso8601(<<?-, rest::binary>>, calendar) do

This comment has been minimized.

@josevalim

josevalim Oct 10, 2017

Member

I think this means we can have this date: --2012-01-01.

@josevalim

This comment has been minimized.

Member

josevalim commented Oct 10, 2017

Definitely moving in the right direction! 👍

@unix_range_microseconds @unix_start..@unix_end
unix_start = -1_000_000 * (315569519999 + @unix_epoch)
unix_end = 1_000_000 * (315569519999 - @unix_epoch)
@unix_range_microseconds unix_start..unix_end
@type year :: 0..9999

This comment has been minimized.

@brodeuralexis

brodeuralexis Oct 12, 2017

Contributor

Should this be -9999..9999 ?

@lexmag lexmag changed the title from [WIP] Support negative dates in Calendar to Support negative dates in Calendar Feb 10, 2018

@@ -254,6 +256,12 @@ defmodule Date do
end
end
def from_iso8601(<<?-, next, rest::binary>>, calendar) when next != ?- do
with {:ok, %{year: year} = date} <- from_iso8601(<<next>> <> rest, calendar) do

This comment has been minimized.

@michalmuskala

michalmuskala Feb 10, 2018

Member

I think we should extract the parsing to a separate function - right now we'll always do an allocation with the "next" bit.

def from_iso8601(<<?-, rest::binary>>, calendar) do
  case parse_iso8601(rest) do
    {year, month, day} -> 
      with {:ok, date} <- new(- year, month, day, Calendar.ISO), do: convert(date, calendar)

    _ -> {:error, :invalid_format}
  end
end

def from_iso8601(string, calendar) do
  case parse_iso8601(rest) do
    {year, month, day} -> 
      with {:ok, date} <- new(year, month, day, Calendar.ISO), do: convert(date, calendar)

    _ -> {:error, :invalid_format}
  end
end

@compile {:inline, parse_iso8601: 1}
defp parse_iso8601(<<year::4-bytes, ?-, month::2-bytes, ?-, day::2-bytes>>) do
  with {year, ""} when year > 0 <- Integer.parse(year),
       {month, ""} <- Integer.parse(month),
       {day, ""} <- Integer.parse(day) do
    {year, month, day}
   end
end

This comment has been minimized.

@lexmag

lexmag Feb 10, 2018

Member

I went with that "next" allocation for simplicity reason.
I believe negative dates are rare enough to prefer less code over minor optimization.

This comment has been minimized.

@michalmuskala

michalmuskala Feb 10, 2018

Member

That's fair

@lexmag lexmag merged commit 2d4be98 into elixir-lang:master Feb 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lexmag lexmag deleted the lexmag:am/negative-calendar branch Feb 11, 2018

treble37 added a commit to treble37/elixir that referenced this pull request Mar 6, 2018

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