-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Xin/support deep levels reserved json name #1191
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
Xin/support deep levels reserved json name #1191
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1191 +/- ##
==========================================
+ Coverage 54.15% 54.46% +0.30%
==========================================
Files 42 42
Lines 4297 4326 +29
==========================================
+ Hits 2327 2356 +29
Misses 1712 1712
Partials 258 258
Continue to review full report at Codecov.
|
@johanbrandhorst , I am working on a test, will push another commit. |
@johanbrandhorst , I finalized the PR. It is ready for review. |
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 looks great! A few comments.
if strings.Contains(fieldName, ".") { | ||
fieldNames := strings.Split(fieldName, ".") | ||
fieldNamesWithCamelCase := make([]string, 0) | ||
for _, oneField := range fieldNames[:len(fieldNames) - 1] { |
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.
Is this slicing necessary?
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.
Good point! Fixing it now.
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, could we see if this fixes the issue before merging?
@johanbrandhorst , good call! I tested on my side using the @brocaar 's example, it worked. |
Thanks for your contribution! |
@johanbrandhorst , anytime! |
Thanks @xin-au for the quick response and fix! 👍 |
@brocaar , anytime! |
* Add message as another argument for supporting deep level reserved json name * Get reserved json name in non direct field of a grpc method * Change variable names based on google naming policy * Change variable names based on google naming policy in test * Reuse variable value * Format code * Update test names * Add doCamelCase and one more test case * Correct a comment in test * Keep all prefix in camel case * Use regular for loop instead of foreach to avoid unnecessary slicing * Update protoc-gen-swagger/genswagger/template_test.go Co-Authored-By: Johan Brandhorst <[email protected]> * Add space in comments * Remove a unnecessary comment Co-authored-by: Johan Brandhorst <[email protected]>
* Add message as another argument for supporting deep level reserved json name * Get reserved json name in non direct field of a grpc method * Change variable names based on google naming policy * Change variable names based on google naming policy in test * Reuse variable value * Format code * Update test names * Add doCamelCase and one more test case * Correct a comment in test * Keep all prefix in camel case * Use regular for loop instead of foreach to avoid unnecessary slicing * Update protoc-gen-swagger/genswagger/template_test.go Co-Authored-By: Johan Brandhorst <[email protected]> * Add space in comments * Remove a unnecessary comment Co-authored-by: Johan Brandhorst <[email protected]>
Supporting deep levels reserved json name. Regarding the issue #1187