-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[usage] Extend method call with order arg #11694
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
Conversation
started the job as gitpod-build-laushinka-incorporate-reverse-sorting-11691.1 because the annotations in the pull request description changed |
b0d2929
to
6e6f103
Compare
6e6f103
to
b2e9b20
Compare
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.
Looks good, just wonder whether we can do better with the type for the new parameter.
e52e626
to
0493b81
Compare
attributionId: string, | ||
startedTimeOrder: SortOrder, | ||
from?: number, | ||
to?: number, |
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.
@laushinka, please introduce ListBilledUsageParams
and wrap the args, for 2 reasons:
- it's likely this args list may be extended, right?
- optional args at the end are simply not working with jsonrpc, because behind the scene there will be another param added (I think it's a cancellation token) which leads to nasty bugs in JS evaluation.
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.
optional args at the end are simply not working with jsonrpc, because behind the scene there will be another param added (I think it's a cancellation token) which leads to nasty bugs in JS evaluation.
Done! Thanks for the tip.
0493b81
to
20324c1
Compare
20324c1
to
aeaa3ae
Compare
Description
Adds the ordering argument to
listBilledUsage
calls.View implementation will be in a separate issue and PR.
Related Issue(s)
Fixes #11691
How to test
listBilledUsage
should be called with the additional param '0'.Release Notes
Documentation
Werft options: