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

Fix typespecs for beginning_of_week and friends #315

Merged
merged 1 commit into from Jun 21, 2017

Conversation

3 participants
@randycoulman
Copy link
Contributor

randycoulman commented Jun 16, 2017

Summary of changes

When running dialyzer against my project, I discovered that the type spec for Timex.beginning_of_week/2 was not correct. The docs state:

The weekstart can between 1..7, an atom e.g. :mon, or a string e.g. “Monday”

But the typespec only allows for Timex.Types.weekday, which is defined to be non_neg_integer.

Further investigation indicates that the same issue affects end_of_week, days_to_beginning_of_week and days_to_end_of_week as well.

Checklist

  • New functions have typespecs, changed functions were updated
  • Same for documentation, including moduledocs
  • Tests were added or updated to cover changes
  • Commits were squashed into a single coherent commit

Details

There's not really a way to write tests for this that I can think of, but before making my changes, running mix dialyze reports a number of warnings, including:

lib/timex.ex:1286: Function days_to_end_of_week/1 has no local return

lib/timex.ex:1286: The call 'Elixir.Timex':days_to_end_of_week(x0@1::any(),'mon')
will never return since the success typing is ({{integer(),integer(),integer()},{byte(),byte(),byte()}} |
{integer(),integer(),integer()} | #{'__struct__':='Elixir.Date' | 'Elixir.DateTime' | 'Elixir.NaiveDateTime',
'calendar':=atom(), 'day':=integer(), 'month':=integer(), 'year':=integer(), 'hour'=>byte(),
'microsecond'=>{char(),0 | 1 | 2 | 3 | 4 | 5 | 6}, 'minute'=>byte(), 'second'=>byte(),
'std_offset'=>integer(), 'time_zone'=>binary(), 'utc_offset'=>integer(),
'zone_abbr'=>binary()},non_neg_integer()) -> non_neg_integer() | {'error',_} and the contract is
('Elixir.Timex.Types':valid_datetime(),'Elixir.Timex.Types':weekday()) -> integer() | {'error',term()}

After making these changes, those two warnings are gone.

Similarly, in my project I was getting these warnings for this code:

def for_current_status(today \\ local_today()) do
  Timex.beginning_of_week(today, :fri)
end
lib/invoice_tracker/default_date.ex:37: Function for_current_status/0 has no local return
lib/invoice_tracker/default_date.ex:37: Function for_current_status/1 has no local return
lib/invoice_tracker/default_date.ex:39: The call 'Elixir.Timex':beginning_of_week(today@1::any(),'fri')
breaks the contract ('Elixir.Timex.Types':valid_datetime(),'Elixir.Timex.Types':weekday()) ->
'Elixir.Timex.Types':valid_datetime() | {'error',term()}

Those warnings are now gone with these changes.

An interesting side note: mix dialyze is only complaining about days_to_end_of_week and not days_to_beginning_of_week. That's because days_to_end_of_week uses :mon as its default weekstart value, whereas days_to_beginning_of_week uses 1 instead. I didn't change this to be consistent, though I could if that's desirable.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.04%) to 67.18% when pulling dc25da5 on randycoulman:fix-weekstart-typespecs into 0b90509 on bitwalker:master.

@bitwalker bitwalker merged commit e8aceb6 into bitwalker:master Jun 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 67.18%
Details
@bitwalker

This comment has been minimized.

Copy link
Owner

bitwalker commented Jun 21, 2017

Thanks!

@randycoulman randycoulman deleted the randycoulman:fix-weekstart-typespecs branch Jun 21, 2017

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