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

new BeAfter matcher for time.Time values #41

Merged
merged 6 commits into from Jul 3, 2014
Merged

Conversation

tve
Copy link
Contributor

@tve tve commented Jul 1, 2014

I created a new matcher for time.Time values. See the tests for simple usage. Please advise whether this is acceptable or whether you'd like modifications or don't like it at all. :-)
(If this looks good, I'll also create a pull request for the docs)

@tve
Copy link
Contributor Author

tve commented Jul 2, 2014

I just added a HaveKeyValue matcher to match a map's key and value. Usage like: Ω(myMap).Should(HaveKeyValue(k, v)). This is better than just testing for Ω(myMap[k]).Should(Equal(v)) in that it provides better error messages by saying whether the key is missing or the value is incorrect.

@onsi
Copy link
Owner

onsi commented Jul 2, 2014

A few thoughts:

I really like HaveKeyValue (and thank you for making sure it could also take matchers). i think it would read a bit better if it was HaveKeyWithValue but other than that, it's ready to be merged in. Any chance you could make it a separate PR?

I think the motivation for BeAftermakes sense but I have two suggestions:

First: unfortunately, I don't think we can change the behavior of format w.r.t to time.Time. For one reason why check out the second comment on issue #37

Instead, I think this matcher should just do its own formatting. Either by adding a format.Time() method or just doing the formatting inline.

Second: BeAfter makes sense, but it might be more consistent and powerful to mirror BeNumerically. What if we had BeTemporally(op, time.Time, ...time.Duration) with these modes:

Ω(t1).Should(BeTemporally(">", t2)
Ω(t1).Should(BeTemporally(">=", t2)
Ω(t1).Should(BeTemporally("=", t2)
Ω(t1).Should(BeTemporally("<=", t2)
Ω(t1).Should(BeTemporally("<", t2)
Ω(t1).Should(BeTemporally("~", t2, time.Millisecond)

Thoughts?

@onsi
Copy link
Owner

onsi commented Jul 2, 2014

And, it goes without saying: thanks for working on these!

@tve
Copy link
Contributor Author

tve commented Jul 2, 2014

I'll make the change to HaveKeyWithValue
I'll ponder the BeTemporally. Makes sense, but it's a bit painful to implement with the time pkg.

@tve
Copy link
Contributor Author

tve commented Jul 2, 2014

How's that? :-)
I reverted the time.Time formatting and will try setting format.UseStringerRepresentation to true, where's that documented, by the way?

@onsi
Copy link
Owner

onsi commented Jul 2, 2014

format.UseStringerRepresentation hasn't been documented yet... I have a few things that need to be backfilled. Will open an issue for it.

@onsi
Copy link
Owner

onsi commented Jul 2, 2014

This PR's getting close. I have one more minor nit before merging in if you don't mind fixing it!

Instead of

func BeTemporally(comparator string, compareTo ...interface{}) OmegaMatcher {
    return &matchers.BeTemporallyMatcher{
        Comparator: comparator,
        CompareTo:  compareTo,
    }
}

which forces a type check at runtime we could enforce the type check at compile time with:

func BeTemporally(comparator string, compareTo time.Time, threshold ...time.Duration) OmegaMatcher {
    return &matchers.BeTemporallyMatcher{
        Comparator: comparator,
        CompareTo:  compareTo,
        Threshold: threshold,
    }
}

Would you mind making that change? Once it's done I think this'll be ready to merge!

@onsi onsi added this to the Version 1.0 milestone Jul 2, 2014
@tve
Copy link
Contributor Author

tve commented Jul 2, 2014

I had wondered about that myself and wasn't sure whether you specifically wanted interface{} on all these params. I'll make the change. After that, additional changes are all yours ;-)

@onsi
Copy link
Owner

onsi commented Jul 2, 2014

Thanks! Sorry for the nitpicking - excited to merge this in!

@tve
Copy link
Contributor Author

tve commented Jul 2, 2014

Nah, no worries, I rather contribute to a project that has a nitpicky committer than someone who merges any garbage in!
I hope to get to this later today.

@tve
Copy link
Contributor Author

tve commented Jul 3, 2014

I changed the args of BeTemporally to time.Time/time.Duration. I left the actual as interface{} and am not sure about the pros/cons of making that a time.Time as well. Happy to make that change too ;-)

@onsi
Copy link
Owner

onsi commented Jul 3, 2014

looks good to me!

onsi added a commit that referenced this pull request Jul 3, 2014
new BeAfter matcher for time.Time values
@onsi onsi merged commit 0d06fd5 into onsi:master Jul 3, 2014
@onsi
Copy link
Owner

onsi commented Jul 3, 2014

also, don't worry about the docs. I'll update the github pages later today/tomorrow.

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

Successfully merging this pull request may close these issues.

None yet

2 participants