Skip to content

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

Conversation

xin-au
Copy link
Contributor

@xin-au xin-au commented Mar 29, 2020

Supporting deep levels reserved json name. Regarding the issue #1187

@codecov-io
Copy link

codecov-io commented Mar 29, 2020

Codecov Report

Merging #1191 into master will increase coverage by 0.30%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
protoc-gen-swagger/genswagger/template.go 57.21% <94.73%> (+1.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48f2e5a...561c516. Read the comment docs.

@xin-au
Copy link
Contributor Author

xin-au commented Mar 29, 2020

@johanbrandhorst , I am working on a test, will push another commit.

@xin-au
Copy link
Contributor Author

xin-au commented Mar 29, 2020

@johanbrandhorst , I finalized the PR. It is ready for review.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a 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] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this slicing necessary?

Copy link
Contributor Author

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.

@xin-au xin-au requested a review from johanbrandhorst March 29, 2020 18:24
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a 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?

@xin-au
Copy link
Contributor Author

xin-au commented Mar 29, 2020

@johanbrandhorst , good call! I tested on my side using the @brocaar 's example, it worked.
@johanbrandhorst @brocaar , sorry for any inconvenience it caused. Welcome any feedback.

@johanbrandhorst johanbrandhorst merged commit 4a27188 into grpc-ecosystem:master Mar 29, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@xin-au
Copy link
Contributor Author

xin-au commented Mar 29, 2020

@johanbrandhorst , anytime!

@brocaar
Copy link
Contributor

brocaar commented Mar 30, 2020

Thanks @xin-au for the quick response and fix! 👍

@xin-au
Copy link
Contributor Author

xin-au commented Mar 30, 2020

@brocaar , anytime!

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* 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]>
pull bot pushed a commit to BuildingRobotics/grpc-gateway that referenced this pull request Apr 29, 2020
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants