-
Notifications
You must be signed in to change notification settings - Fork 9
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 gateway_api::Duration type to support GEP-2257 Durations #45
Conversation
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.
As @kflynn mentioned, we were on a Slack call while he was working this so I probably should have voiced some of these nits then (apologies Flynn 😅).
In any case, it all obviously looks good. I've added a few suggestions, but absolutely none of them are like hard objections. Please feel absolutely free to accept or reject any of them as you see fit.
Signed-off-by: Flynn <[email protected]>
…tdDurations Co-authored-by: Mark S. <[email protected]> Signed-off-by: Flynn <[email protected]>
…or consistency Co-authored-by: Mark S. <[email protected]> Signed-off-by: Flynn <[email protected]>
…sing fails Co-authored-by: Mark S. <[email protected]> Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
Signed-off-by: Flynn <[email protected]>
6cbc73b
to
cb33b4e
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 great! very thorough, lots of comments, very nice. Thanks for adding this!
I took a first pass of review, but I will be unavailable for most of today, so I didn't get through everything: my goal will be to review again tonight.
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.
Once we get the current comments resolved, then 👍
Thanks very much for adding this @kflynn! Appreciate that there are lots of comments and that you covered all the upstream test cases 🙇
Co-authored-by: Mark S. <[email protected]> Signed-off-by: Flynn <[email protected]>
Co-authored-by: Shane Utt <[email protected]> Signed-off-by: Flynn <[email protected]>
Co-authored-by: Shane Utt <[email protected]> Signed-off-by: Flynn <[email protected]>
Co-authored-by: Shane Utt <[email protected]> Signed-off-by: Flynn <[email protected]>
Co-authored-by: Shane Utt <[email protected]> Signed-off-by: Flynn <[email protected]>
Co-authored-by: Shane Utt <[email protected]> Signed-off-by: Flynn <[email protected]>
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.
/approve
/lgtm
gateway_api::Duration
is a duration type where parsing and formatting obey GEP-2257. This PR adds the type, unit tests, and an example.Reviewers: reviewing commit by commit might be helpful, but unlike most of my PRs, it's really not that big a deal this time around.
(Many thanks to @shaneutt and @the-wondersmith for answering Rust questions. 😂)
Signed-off-by: Flynn [email protected]