-
Notifications
You must be signed in to change notification settings - Fork 66
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
add increment and decrement public API to Meter #132
Conversation
motivation: expose increment and decrement public APIs changes: * add increment and decrement public API to Meter * improve the implementation of AccumulatingMeter and TestMeter * add tests for new APIs * refactor/modernize other tests
guard !amount.isNaN else { | ||
return | ||
} | ||
// - cannot increment by infinite quantities | ||
guard !amount.isInfinite else { | ||
return | ||
} | ||
// - cannot increment by negative values | ||
guard amount.sign == .plus else { | ||
return | ||
} | ||
// - cannot increment by zero | ||
guard !amount.isZero else { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just silently return here or give the user any signal that he used an invalid value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could make this throwing, but not sure if that make the call sites more cumbersome, note this is the default implementation, so in most cases the actual backend will drive the behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't throwing makes sense here since it makes the API very awkward to use. I was more thinking if we should precondition this but that's probably also not a great idea. Overall, I am fine with just dropping them on the ground but wanted to raise the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could assert
on this to ensure that debug builds fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is a good idea... but only when passing negative values? the rest may happen when the value is programmatically computed and that's fine - we dont want it to impact the meter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
motivation: expose increment and decrement public APIs
changes: