-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
runtime: add support for time types in query parameters #693
runtime: add support for time types in query parameters #693
Conversation
Adds support for parsing the google.protobuf.Duration as well as native *time.Time and *time.Duration types in url query parameters. Helps grpc-ecosystem#400.
} | ||
*t = d | ||
return nil | ||
} |
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.
This part may be controversial, but I think considering this does not add any explicit dependencies on any gogoproto packages, I argue that this should be acceptable.
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.
Can you elaborate? What could be considered controversial?
t.Errorf("runtime.PopulateQueryParameters(msg, %v, utilities.NewDoubleArray(nil)) = %v; want %v", spec.values, got, want) | ||
} | ||
} | ||
} |
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.
This needs to be a separate test because golang/protobuf/proto.Clone
panics when using *time.Time
in the proto message.
Codecov Report
@@ Coverage Diff @@
## master #693 +/- ##
==========================================
+ Coverage 56.47% 56.53% +0.05%
==========================================
Files 30 30
Lines 3005 3032 +27
==========================================
+ Hits 1697 1714 +17
- Misses 1145 1151 +6
- Partials 163 167 +4
Continue to review full report at Codecov.
|
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
…em#693) Adds support for parsing the google.protobuf.Duration as well as native *time.Time and *time.Duration types in url query parameters. Helps grpc-ecosystem#400.
Adds support for parsing the google.protobuf.Duration as well as native *time.Time and *time.Duration types in url query parameters.
Helps #400. Fixes gogo/protobuf#420.