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

Be date spec comparator #226

Merged
merged 33 commits into from Nov 30, 2017
Merged

Be date spec comparator #226

merged 33 commits into from Nov 30, 2017

Conversation

@treble37
Copy link
Contributor

@treble37 treble37 commented Aug 22, 2017

For #111

  • Please namespace all the modules connected with this feature to separate module.
    Let's say ESpec.DatesTimes. And place the code into dates_times directory.
  • Please add specs for different cases. Place them to 'spec/dates_times' folder.
  • Address Zeeker's recommendations
  • Please add README section which describes the cases
  • Return {result, delta} (#226 (comment)) ...?
  • Also, could we think about improvements of be(:>, ...) and be(:<, ...) mathers? So they use the same comparison logic.
  • Misc: Add extra error tests in dates_times/be_close_to.exs
  • use the Comparator.diff when dates are passed to the be assertion
    (#226 (comment))
  • Add case for single keyword list (see #226 (comment) and #226 (comment))
  • clean up spec docs
  • Add equivalent ExUnit assertions...
  • update README with the new keyword list args in "be" assertion
@antonmi
Copy link
Owner

@antonmi antonmi commented Oct 3, 2017

Hi, @treble37 !
Sorry, I may have missed smth, but what is the status of this PR?
Is it ready for merging or you want to add smth?
Thanks!

@treble37
Copy link
Contributor Author

@treble37 treble37 commented Oct 4, 2017

Hi @antonmi, just a few things...

  1. I was hoping for feedback on the handling of the timezones wrt DateTime tests if someone is able... essentially what I did was backport a fair amount of the Elixir 1.5 functionality to make it compatible down to 1.3...
  2. Shall I also add the corresponding ExUnit tests as well in this PR?
  3. Based on the feedback, I might make some changes then, depending....
@antonmi
Copy link
Owner

@antonmi antonmi commented Oct 4, 2017

Oh, sorry!
I will review soon

@antonmi
Copy link
Owner

@antonmi antonmi commented Oct 4, 2017

Hi, @treble37 !
Sorry again for such late review!
There are a few comments:

  1. Please namespace all the modules connected with this feature to separate module.
    Let's say ESpec.DatesTimes. And place the code into dates_times directory.
    So the structure will be:
    lib/
    dates_times/
    comparator.ex (call the module ESpec.DatesTimes.Comparator, no need to separate Diff module)
    date_time_protocol.ex (So module will be ESpec.DatesTimes.DateTimeProtocol )
    types.ex (ESpec.DateTimes.Types )
    impl/
    date.ex
    datetime.ex
    naive_datetime.ex
    time.ex
    extensions/
    calendar_extension.ex
    datetime_extension.ex
    integer_extension.ex

  2. Please add specs for different cases. Place them to 'spec/dates_times' folder.

  3. Please add README section which describes the cases

Also, could we think about improvements of be(:>, ...) and be(:<, ...) mathers? So they use the same comparison logic.

Thank you!

@sascha-wolf
Copy link
Contributor

@sascha-wolf sascha-wolf commented Oct 5, 2017

@antonmi, @treble37 are you okay with me chiming in on the changes and also providing a review? I don't want to force myself into this. 😉

@antonmi
Copy link
Owner

@antonmi antonmi commented Oct 5, 2017

@zeeker you are very welcome!

@treble37
Copy link
Contributor Author

@treble37 treble37 commented Oct 5, 2017

Hi @zeeker - your input would be welcome, I have a feeling there are things that can be improved, so please review away and don't be shy!

@sascha-wolf
Copy link
Contributor

@sascha-wolf sascha-wolf commented Oct 5, 2017

I will, lot's of meetings today! 😄

Copy link
Contributor

@sascha-wolf sascha-wolf left a comment

I'm asking myself if there is no native way of doing this. DateTime.diff/3 only works for seconds and below but isn't there something else?

Most of the stuff is of cosmetic nature, feel free to dismiss those.

end
end

defp do_diff(a, a, type), do: zero(type)

This comment has been minimized.

@sascha-wolf

sascha-wolf Oct 6, 2017
Contributor

The alignment of the do: seems to be off here.

This comment has been minimized.

@treble37

treble37 Oct 8, 2017
Author Contributor

Adjusted in accordance with #226 (comment) (If you use the do: syntax with functions and the line that makes up the function body is long, put the do: on a new line indented one level more than the previous line. )

@@ -5,6 +5,12 @@ defmodule ESpec.Assertions.BeCloseTo do
it do: expect(2).to be_close_to(1, 3)
"""
use ESpec.Assertions.Interface
alias ESpec.Comparator.Diff

This comment has been minimized.

@treble37

treble37 Oct 8, 2017
Author Contributor

Space added between use and alias

alias ESpec.Comparator.Diff

defp match(subject, [value, {granularity, delta}] = data) do
result = abs(Diff.diff(subject, value, granularity)) <= delta

This comment has been minimized.

@sascha-wolf

sascha-wolf Oct 6, 2017
Contributor

One could write that as follows but I'm not sure if that's a clearer approach; I'll mention it for completeness sake.

result =
  subject
  |> Diff.diff(value, granularity)
  |> abs()
  |> Kernel.<=(delta)

This comment has been minimized.

@treble37

treble37 Oct 16, 2017
Author Contributor

thanks, I like this and will use it.


defp match(subject, [value, {granularity, delta}] = data) do
result = abs(Diff.diff(subject, value, granularity)) <= delta
{result, result}

This comment has been minimized.

@sascha-wolf

sascha-wolf Oct 6, 2017
Contributor

Instead of return the result boolean twice you could return the result and the real delta. I'm aware that the original implementation didn't do that but I think it would be a great improvement!

Then you could use this in the success_message and/or error_message to inform the user about the real delta which might help in finding the issue.

This comment has been minimized.

@treble37

treble37 Oct 30, 2017
Author Contributor

Preliminary interface for providing a real computed delta with an error message added...

end
defp do_diff(a, b, :years) do
diff_years(a, b)
end

This comment has been minimized.

This comment has been minimized.

@treble37

treble37 Oct 8, 2017
Author Contributor

Adjusted in accordance with #226 (comment) (If you use the do: syntax with functions and the line that makes up the function body is long, put the do: on a new line indented one level more than the previous line. )

if a > b do
diff_years(end_date, start_date, 0)
else
diff_years(start_date, end_date, 0) * -1

This comment has been minimized.

@sascha-wolf

sascha-wolf Oct 6, 2017
Contributor

Maybe also call this do_diff_years to match the approach of the diff_months implementation?

This comment has been minimized.

@treble37

treble37 Oct 16, 2017
Author Contributor

Will change, thanks!

diff_years(a, b)
end
defp do_diff(_, _, granularity) when not granularity in @units,
do: {:error, {:invalid_granularity, granularity}}

This comment has been minimized.

@treble37

treble37 Oct 8, 2017
Author Contributor

Adjusted in accordance with #226 (comment) (If you use the do: syntax with functions and the line that makes up the function body is long, put the do: on a new line indented one level more than the previous line. )

defp do_diff(_, _, granularity) when not granularity in @units,
do: {:error, {:invalid_granularity, granularity}}

defp diff_years(a, a), do: 0

This comment has been minimized.

@sascha-wolf

sascha-wolf Oct 6, 2017
Contributor

Is there actually a case where this will happen? Wouldn't this have been caught on line 23? Same for diff_months.

This comment has been minimized.

@treble37

treble37 Oct 16, 2017
Author Contributor

great catch Zeeker, will change

12
end
(year_diff*12)+month_diff
end

This comment has been minimized.

@sascha-wolf

sascha-wolf Oct 6, 2017
Contributor

I'm not sure, but wouldn't it be possible to reuse the diff_years function to reduce complexity in do_diff_months?

This comment has been minimized.

@treble37

treble37 Oct 16, 2017
Author Contributor

@zeeker - If I'm understanding you correctly, the current implementation of diff_years that I think you're talking about (listed below) assumes a and b are date(times) converted to gregorian microseconds rather than integer years.....

  defp diff_years(a, b) do
    {start_date, _} = :calendar.gregorian_seconds_to_datetime(div(a, 1_000*1_000))
    {end_date, _} = :calendar.gregorian_seconds_to_datetime(div(b, 1_000*1_000))
    if a > b do
      do_diff_years(end_date, start_date, 0)
    else
      do_diff_years(start_date, end_date, 0) * -1
    end
  end
@antonmi
Copy link
Owner

@antonmi antonmi commented Oct 6, 2017

@zeeker , thank you very much for detailed comments!

@treble37 treble37 force-pushed the treble37:be-date-spec-comparator branch from f0196d5 to 04c7ef7 Oct 8, 2017
@treble37 treble37 force-pushed the treble37:be-date-spec-comparator branch from 9b2bbed to 5e4791b Oct 22, 2017
@treble37
Copy link
Contributor Author

@treble37 treble37 commented Nov 5, 2017

Ok @antonmi (and @zeeker), this PR is getting close to done (other than some minor documentation cleanup), but I would like some feedback regarding your comment:

Also, could we think about improvements of be(:>, ...) and be(:<, ...) mathers? So they use the same comparison logic.

Commit 9b5d91d is my attempt to do this. What do you think of this API? If you're fine with it, then I will add the necessary tests to implement.

@antonmi
Copy link
Owner

@antonmi antonmi commented Nov 5, 2017

Hi, @treble37 !

Thanks for your awesome work!
About be assertion:

What I meant was use the Comparator.diff when dates are passed to the be assertion:

Like this:

  defp match(%Date{} = subject, [op, %Date{} = val]), do: match_date_times(subject, [op, val])
  defp match(%Time{} = subject, [op, %Time{} = val]), do: match_date_times(subject, [op, val])
  defp match(%DateTime{} = subject, [op, %DateTime{} = val]), do: match_date_times(subject, [op, val])
  defp match(%NaiveDateTime{} = subject, [op, %NaiveDateTime{} = val]), do: match_date_times(subject, [op, val])

  defp match_date_times(subject, [op, val]) do
    delta = subject |> Comparator.diff(val, :seconds)

    result = apply(Kernel, op, [delta, 0])
    {result, result}
  end

But your API implementation is also very cool!
So, my suggestion: Let's keep both!

@treble37 treble37 force-pushed the treble37:be-date-spec-comparator branch from 9b5d91d to 44e885e Nov 6, 2017
@sascha-wolf
Copy link
Contributor

@sascha-wolf sascha-wolf commented Nov 6, 2017

Hey @treble37,

I can only join antomni in saying thank you for your work and patience, and I'm glad I could be of some help.

I just have one final suggestion. In addition to the current be Implementation, you could add a case for a single keyword list, similar to how have can now receive values.

This would allow for the following syntax:

expect date |> to(be :>, other_date, years: 1)

I'll link in a second, I'm on mobile right now.

@sascha-wolf
Copy link
Contributor

@sascha-wolf sascha-wolf commented Nov 6, 2017

See commit ae7a4ca for details of what I mean.

@treble37
Copy link
Contributor Author

@treble37 treble37 commented Nov 7, 2017

ok @zeeker, thank you for your review and suggestion(s) as always; regarding
your comment #226 (comment) this commit (ec3986d) enables it

I will work on adding the equivalent ExUnit tests and doing a final cleanup in the next 2 weeks or so then...

treble37 added 11 commits Aug 7, 2017
For issue #111 "Add Calendar expectation"
This reverts commit e694572.

Import Timex logic to compare Date types and use a different API for
Date type comparison
Change ESpec.Protocol to ESpec.DateTimeProtocol
Using the to_gregorian_microseconds interface - This is a bit misleading
since we're really just converting the time to microseconds, it has
nothing to do with gregorian
Add tests to be able to compare microsecond granularity for DateTime and
NaiveDateTime
Avoid warnings such as: "undefined protocol function to_gregorian_seconds/1
(for protocol ESpec.DateTimeProtocol) lib/espec/datetime/datetime.ex:1"
Tests passing with Elixir 1.5.1 only
Tests passing for Elixir 1.4.5 (and 1.5.1)

This commit is to backport Elixir 1.5.1 functionality via the
Calendar.ISO.Extension module
This commit is to backport Elixir 1.5.1 functionality via the
DateTime.Extension and Integer.Extension modules
Let's not rely on the version of Elixir to decide what method to call.
Instead, we'll use one common method for all. Once ESpec migrates to
supporting >= Elixir 1.5.x, then we can probably remove some of this
supporting code.
treble37 added 20 commits Oct 8, 2017
Move this functionality into its own module, Espec.DatesTimesComparator
Help the user hunt down errors by providing the actual computed delta
Move file into dates_times folder for organizational clarity
Use the Comparator.diff when dates are passed to the be assertion. This
lets us have the same comparison logic as the be_close_to assertion(s).
Enable testing syntax such as `expect date |> to(be :>, other_date, years: 1)`. Also, update tests
to accomodate this new syntax.
Move one of the context descriptions in the describe text to avoid
unneccessary block nesting
The dates_times files deal with assertions for Elixir's
Date/DateTime/NaiveDateTime/Time types, so move it here.
Also, fix typo in error message
Since we ported over Types from the Timex hex package, let's remove the
unused types
@treble37 treble37 force-pushed the treble37:be-date-spec-comparator branch from 9d14fca to a99a448 Nov 29, 2017
@treble37
Copy link
Contributor Author

@treble37 treble37 commented Nov 29, 2017

ok @antonmi and @zeeker, this PR is now updated with the functionality and I addressed yours and Zeeker's comments (which I put in the checklist up top #226 (comment)), so I guess it's ready to merge unless you have any other comments?

I can bump the version to 1.4.7 1.5.0 and edit the CHANGELOG if you're ok with everything.

@sascha-wolf
Copy link
Contributor

@sascha-wolf sascha-wolf commented Nov 29, 2017

Awesome work @treble37! Semantic versioning would suggest to bump the version to 1.5.0 since you introduce a backwards compatible feature. Those are just my two cents though. 🙂

@antonmi
Copy link
Owner

@antonmi antonmi commented Nov 29, 2017

Absolutely awesome!
Thank you @treble37 !
I will review and merge soon!
Thank you @zeeker for comments!

treble37 added 2 commits Nov 29, 2017
Update changelong in README
Remove some minor warning messages as tests are run
@treble37
Copy link
Contributor Author

@treble37 treble37 commented Nov 29, 2017

Of course you are right @zeeker - version bumped to 1.5.0 (maybe I shouldn't have had that last 🍷 😉 )

@antonmi antonmi merged commit ce848f5 into antonmi:master Nov 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@antonmi
Copy link
Owner

@antonmi antonmi commented Nov 30, 2017

ESpec 1.5.0 is live!
Thanks again to everyone!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.