-
Notifications
You must be signed in to change notification settings - Fork 1.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
Encode arrays correctly in query params. #1754
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1754 +/- ##
==========================================
- Coverage 71.51% 71.45% -0.06%
==========================================
Files 178 178
Lines 13775 13780 +5
==========================================
- Hits 9851 9847 -4
- Misses 3312 3321 +9
Partials 612 612
Flags with carried forward coverage won't be shown. Click here to find out more.
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.
Thanks for making this PR. I guess picking to reuse the same key for all slice values (instead of key[]
or key.<N>
or any other convention) is better than the current behavior (#1713, #878)...
Can you please add some tests in your PR? And also you probably don't need reflection, you should be able to get by with a simple type assertion that v
is []interface{}
?
@na-- I dont think go allows type assertion on []interface{}, Although it allows type assertions on []string though. |
Ah, yeah, fair enough... 🤦♂️ Though the rest of my request still stands - please add some tests, and also you probably don't need to check for |
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.
@noelzubin Thanks for the PR. Are you available to finish it? We'd like to have this merged for v0.33.0. I think it would just need a test and maybe disabling the exhaustive
linter if we can't avoid the reflection.
If we don't hear from you by sometime next week or if you're not available, that's OK too, and we can proceed to do the leftover changes.
Fixed in #2060. Thanks for your contribution @noelzubin! We'll make sure to thank you in the release notes :) |
Closes #1713