-
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
fix query params not populate if method is post #939
fix query params not populate if method is post #939
Conversation
Hi @mingqing, thanks for your pull request. The failing CI test is because you need to regenerate the example files. Please see CONTRIBUTING.md in the root of the repo for a docker one liner. Also, could you add a test that fails before this change and works after it? Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #939 +/- ##
==========================================
- Coverage 53.84% 53.79% -0.06%
==========================================
Files 40 40
Lines 3965 3969 +4
==========================================
Hits 2135 2135
- Misses 1634 1638 +4
Partials 196 196
Continue to review full report at Codecov.
|
@johanbrandhorst I regenerated the file and added it, while passing the CI test. |
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 still doesn't contain the test I requested. Please see the integration test folder for an example of a test. I would like you to add a new test case that exercises this new branch in the generated code please.
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.
The latest update confuses me - I encourage you to add a test case before anything else.
Thanks for the explanation, however, we still need a new test case that was failing before this change and is now passing. Do you need some help to write the test? |
Is the test case added in examples/integration/integration_test.go? I will go add. |
Yes, that would be great, thanks. |
Thanks for adding the tests, I took the liberty of trying to run the tests without the changes applied and was pleased to see that the |
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 the latest slew of changes, just a new small pointers then we can merge 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.
This looks great now, thanks a lot!
Thanks for your contribution! |
* fix query params not populate if method is post * regenerate example files. * fix go.mod * use req.From instead of req.PostForm to avoid missing url params in post method * regenerate example files * add test case and regenerate example files * adjust to use subtests and remove confused variable * use an explicit test name instead of the auto index * rename local var url to apiURL avoid conflict with net/url
for http method post and content-type is application/x-www-form-urlencoded, the query parameters will be in Request.Form or Request.PostForm.