-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: add enumAsInts flag #1186
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
johanbrandhorst
left a comment
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.
Some of these statements are very similar, maybe we could break them out into a new function instead?
Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com>
|
@johanbrandhorst resolved :) |
Codecov Report
@@ Coverage Diff @@
## master #1186 +/- ##
==========================================
- Coverage 54.23% 54.15% -0.09%
==========================================
Files 42 42
Lines 4270 4297 +27
==========================================
+ Hits 2316 2327 +11
- Misses 1695 1712 +17
+ Partials 259 258 -1
Continue to review full report at Codecov.
|
johanbrandhorst
left a comment
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.
That's great, could we also just add a test for this behaviour? There should be some examples in the existing tests for how to test flags.
johanbrandhorst
left a comment
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.
Thanks for your contribution!
|
@johanbrandhorst @rowe0x The enums are generated with an array of strings rather than an array of ints, which down-stream breaks swagger generators. Was this really the expected behavior? |
|
It's an optional flag, if it breaks your generators just don't set it. Or am I misunderstanding something? |
|
Absolutely, it's not an issue in the installed-base sense. But for the optional feature itself, I'm not sure how to use it since it's not a format supported by swagger, or at least by the "standard". I'd love to have enums as ints, and I was pleased to see this land 6 days before I wanted it :-) but the downstream support doesn't seem to be there. And if it did, it might prefer an array of int rather than an array of strings ("0", "1", etc). |
|
Ok, I see what you're saying. Regardless, clearly some users wanted it and here we are 🤷♂️. |
* feat: add enumAsInts flag * Update protoc-gen-swagger/genswagger/template.go Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com> * fix: typo * feat: add comment * feat: add test Co-authored-by: Johan Brandhorst <johan.brandhorst@gmail.com>
* feat: add enumAsInts flag * Update protoc-gen-swagger/genswagger/template.go Co-Authored-By: Johan Brandhorst <johan.brandhorst@gmail.com> * fix: typo * feat: add comment * feat: add test Co-authored-by: Johan Brandhorst <johan.brandhorst@gmail.com>
fix #1177