-
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
improve WKT handling in gateway and openapi output #404
Conversation
refs #400 |
@@ -56,3 +60,17 @@ func Uint32(val string) (uint32, error) { | |||
} | |||
return uint32(i), nil | |||
} | |||
|
|||
// Timestamp converts the given RFC3339 formatted string into a timestamp.Timestamp. | |||
func Timestamp(val string) (*timestamp.Timestamp, error) { |
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.
When would these functions be called?
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.
these are dropped in as 'runtime.Timestamp' calls via ConvertFuncExpr
} | ||
|
||
// Duration converts the given string into a timestamp.Duration. | ||
func Duration(val string) (*duration.Duration, error) { |
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 also add a couple of simple tests to validate this?
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 was considering that but figured this code was well-tested in jsonpb
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've been thinking on this for a bit. I think you're just exposing helper methods against jsonpb. That should mean they are well tested. If these were to become more complex then I would want tests but they are just so imple I think we should leave it.
Any update on this? |
@AlekSi this is waiting on some additional test coverage. I've been bogged down with ${dayjob} and haven't written said tests. This could land sooner if those were contributed. |
I'm facing the similar problem about WKT handling now, so I would like to take over this task. |
I think adding a convert_test.go file would be appropriate. |
} | ||
|
||
// Duration converts the given string into a timestamp.Duration. | ||
func Duration(val string) (*duration.Duration, error) { |
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've been thinking on this for a bit. I think you're just exposing helper methods against jsonpb. That should mean they are well tested. If these were to become more complex then I would want tests but they are just so imple I think we should leave it.
@ajalab I think I was wrong to request testing on those methods and block. I'm going to merge this. If you have any more needs beyond this can you send another PR? |
This improves Timestamp and Duration support in field marshaling and openapi/swagger output.