Skip to content

Add assert/opt for creating gocmp.Options#62

Merged
vdemeester merged 1 commit into
gotestyourself:masterfrom
dnephin:gocmp-opts
Mar 21, 2018
Merged

Add assert/opt for creating gocmp.Options#62
vdemeester merged 1 commit into
gotestyourself:masterfrom
dnephin:gocmp-opts

Conversation

@dnephin
Copy link
Copy Markdown
Member

@dnephin dnephin commented Feb 17, 2018

assert/opt is for creating a more concise and convenient API for the common cases where gocmp.Option are used in DeepEqual.

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@dnephin dnephin force-pushed the gocmp-opts branch 3 times, most recently from 604c7d7 to bedec6e Compare March 4, 2018 21:43
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Mar 8, 2018

After some discussion on a go-cmp issue and more investigation into the cmpopts package I think the path filter part of this should be dropped in favor of exporting what already exists in cmpopts. Hopefully that will be accepted.

So this PR will only add CmpTime and CmpDuration, which might need to be renamed to make it clear they compare within a threshold.

@dnephin dnephin changed the title WIP: Add assert/opt for creating gocmp.Options Add assert/opt for creating gocmp.Options Mar 13, 2018
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Mar 13, 2018

I removed the path matching from this PR hoping that something makes it into cmpopts (google/go-cmp#76).

This is now just the functions for comparing Time and Duration deltas are within some threshold.

Copy link
Copy Markdown
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

still LGTM :tiger:

@vdemeester vdemeester merged commit 5743e1c into gotestyourself:master Mar 21, 2018
@dnephin dnephin deleted the gocmp-opts branch March 21, 2018 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants