Skip to content

Add calendar callback months_in_year/1 #7417

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

Merged
merged 1 commit into from
Mar 5, 2018
Merged

Add calendar callback months_in_year/1 #7417

merged 1 commit into from
Mar 5, 2018

Conversation

kipcole9
Copy link
Contributor

@kipcole9 kipcole9 commented Mar 4, 2018

Not all calendars have a constant 12 months in
a year. This function returns the number of
months for a given year. Calendar.ISO will return
12 for all years.

@josevalim
Copy link
Member

We should also add a function in Date that takes the year+calendar and invoke it. Similar to days_in_month: https://github.com/elixir-lang/elixir/blob/master/lib/elixir/lib/calendar/date.ex#L171-L177

Other than that, this looks beautiful.

Not all calendars have a constant 12 months in
a year.  This function returns the number of
months for a given year. Calendar.ISO will return
12 for all years.

Add Date.days_in_month/1
Date.days_in_month/1 takes a Date.t and
delegates to the Date's calendar to invoke
days_in_month/1 in the calendar module.
@kipcole9
Copy link
Contributor Author

kipcole9 commented Mar 4, 2018

PR is updated to reflect the addition of Date.months_in_year/1

@josevalim josevalim merged commit d91aed2 into elixir-lang:master Mar 5, 2018
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@@ -176,6 +176,23 @@ defmodule Date do
calendar.days_in_month(year, month)
end

@doc """
Returns the number of days in the given `date` month.

Choose a reason for hiding this comment

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

Shouldn't it be "Returns the number of months in the given year"?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch @David-Gil, fixed in 7d621f6 💟

Choose a reason for hiding this comment

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

Thank you! 😄 I didn't expect to contribute anything to Elixir 😅

@josevalim
Copy link
Member

josevalim commented Jun 28, 2018 via email

whatyouhide added a commit that referenced this pull request Jun 28, 2018
@@ -232,6 +234,22 @@ defmodule Calendar.ISO do
def days_in_month(_, month) when month in [4, 6, 9, 11], do: 30
def days_in_month(_, month) when month in 1..12, do: 31

@doc """
Copy link
Member

@fertapric fertapric Jun 28, 2018

Choose a reason for hiding this comment

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

What do you think about using the module variable in the @doc and @spec?

@doc """
Returns how many months there are in the given year.

## Example

    iex> Calendar.ISO.months_in_year(2004)
    #{@months_in_year}

"""
@impl true
@since "1.7.0"
@spec months_in_year(year) :: unquote(@months_in_year)
def months_in_year(_year) do
  @months_in_year
end

or maybe just remove the module variable and use 12 directly like it's done in days_in_month/2.

@doc """
Returns how many months there are in the given year.

## Example

    iex> Calendar.ISO.months_in_year(2004)
    12

"""
@impl true
@since "1.7.0"
@spec months_in_year(year) :: 12
def months_in_year(_year), do: 12

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me too!

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.

5 participants