X Tutup
The Wayback Machine - https://web.archive.org/web/20201023085756/https://github.com/deepmap/oapi-codegen/pull/166
Skip to content
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

Wrap Chi server and pass parameters as function params #166

Merged
merged 5 commits into from Jul 20, 2020

Conversation

@qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Apr 2, 2020

Wrap the Chi server in the same way it is being done with Echo:

  • Pass in mandatory path arguments as function arguments
  • Pass in query, header and cookie params struct only if there is at least on parameter of these types.

This way the resulting code is more symmetric with Echo and path parameters can be obtained with consulting opaque context values.

The http.ResponseWriter and http.Request are still passed to the server interface because the server is still responsible of writing the response and not all entities are yet managed by the auto generated code, such as decoding the request.Body.

@dannymidnight
Copy link
Contributor

@dannymidnight dannymidnight commented May 7, 2020

This is something that's come up in the past #56 (comment) and looks like a decent change 👍

@deepmap-marcinr deepmap-marcinr merged commit e232d16 into deepmap:master Jul 20, 2020
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@pgolm
Copy link
Contributor

@pgolm pgolm commented Jul 24, 2020

Awesome improvement 👍 . But unfortunately this is a breaking change. I'm not sure if this project uses semver, but it would be nice if you could release this as major change instead as patch the next time.

@deepmap-marcinr
Copy link
Contributor

@deepmap-marcinr deepmap-marcinr commented Jul 24, 2020

@ashwinrs
Copy link
Contributor

@ashwinrs ashwinrs commented Aug 10, 2020

It broke our pipeline as well. We should try and avoid breaking changes to be introduced without updating the major version. Or find another alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.
X Tutup