Skip to content

Conversation

@vfrank66
Copy link
Contributor

No description provided.

Copy link
Contributor

@evan-gordon evan-gordon left a comment

Choose a reason for hiding this comment

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

Overall looks good! Mostly just a few cosmetic comments.

var includedIn bool

// Handle null bounds
if result.IsNull(leftStart) || result.IsNull(leftEnd) || result.IsNull(rightStart) || result.IsNull(rightEnd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this not work in the case of Intverval(null, 1] and Interval[5, null) where we can definitively say that one is not included in the other? Consider adding a test for this sort of a case.


includedIn = (startComp == leftBeforeRight || startComp == leftEqualRight) &&
(endComp == leftBeforeRight || endComp == leftEqualRight)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use a helper method similar to

func arithmetic[t float64 | int64 | int32](m model.IBinaryExpression, l, r t) (result.Value, error) {
here.

}

// Check units match
if leftStartQty.Unit != rightStartQty.Unit || leftEndQty.Unit != rightEndQty.Unit {
Copy link
Contributor

Choose a reason for hiding this comment

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

mind using the ucum lib here? that should rather easily allow for handling this case.

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