Adds support for common operators for time.Time #256
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
Awesome. Thanks. Will try to review on weekend. |
…e and time.Duration to be passed to the operator fns.
…cker return types as they were incorrect.
Contributor
Author
|
Sweet thanks! I added tests to expr_test.go this morning and found I needed to update the checker for this to be fully functional. I think it is pretty close. I'm happy to fix up anything that I missed. Thanks for making this library. :) |
antonmedv
reviewed
Sep 24, 2022
| } | ||
| } | ||
| return false | ||
| return isInterface(t) |
Member
There was a problem hiding this comment.
Can we add test for this part?
Contributor
Author
|
bump. Are there any other areas that need attention? Is there anything blocking accepting this PR? |
antonmedv
approved these changes
Oct 3, 2022
Member
|
Awesome work) really nice. Also will be cool to see some docs. |
Contributor
Author
|
Added some docs with #257. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should resolve the following issues.
#73
#203
#227
Before starting this PR, I attempted to add support for time operators, but ended up with an impossible state b/c I cannot support
interface{} -OP- interface{}without losing the built-in functionality of the operators. The only option I could see was to copy/paste/extend /vm/helpers.go.I decided to search the issues and found that others have run into this too.
With this PR, the operators
==, >=, <=, <, >are included by default allowing users to comparetime.Timeinstances, returning bool.The
+operator adds atime.Durationtotime.Time, returningtime.TimeThe
-operator returns a duration representing the diff between twotime.Timeinstances.Please lmk if there are any concerns with the implementation. Please lmk if you see any gaps in the tests. I'll be happy to fix it up.
:)