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

Pass .Required to BindStyledParameterWithLocation and BindStyledParameter #1315

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

renom
Copy link
Contributor

@renom renom commented Oct 11, 2023

Fixes #954
Works along with oapi-codegen/runtime/pull/14

@jamietanna
Copy link
Member

@renom mind updating the code to follow the changes introduced in https://github.com/oapi-codegen/runtime/releases/tag/v1.1.0 for this? 🙏 Or are you happy with #1353 taking this?

@renom
Copy link
Contributor Author

renom commented Nov 14, 2023

It's okay, I can update the code. I have a few questions though. How do I know if the code is updated right? Will it be enough to update runtime to v1.1.0 and check that cmd/oapi-codegen builds fine?

@jamietanna
Copy link
Member

Thank you!

Yes, exactly - once bumping runtime, as long as the new code compiles, that will at least give us confidence that it's wired in correctly.

We may want to add an integration test (in https://github.com/deepmap/oapi-codegen/blob/master/internal/test/parameters/parameters_test.go) to validate that the underlying issue in #954 is resolved

@renom
Copy link
Contributor Author

renom commented Nov 14, 2023

I updated runtime to v1.1.0, the code builds fine on my laptop. Can it be merged now?

@@ -27,7 +27,7 @@ func (siw *ServerInterfaceWrapper) {{$opid}}(c *fiber.Ctx) error {
}
{{end}}
{{if .IsStyled}}
err = runtime.BindStyledParameter("{{.Style}}",{{.Explode}}, "{{.ParamName}}", c.Params("{{.ParamName}}"), &{{$varName}})
err = runtime.BindStyledParameter("{{.Style}}", {{.Explode}}, {{.Required}}, "{{.ParamName}}", c.Params("{{.ParamName}}"), &{{$varName}})
Copy link
Member

Choose a reason for hiding this comment

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

I think this will fail now, as we've introduced a new function BindStyledParameterWithOptions - mind refactoring these templates to use that new function now?

Copy link
Member

@jamietanna jamietanna left a comment

Choose a reason for hiding this comment

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

Thank you 👏🏽 well also need to run make generate and then commit the changes

@renom renom marked this pull request as ready for review November 14, 2023 18:58
@renom
Copy link
Contributor Author

renom commented Nov 14, 2023

Ready to be merged.

@jamietanna jamietanna added bug Something isn't working enhancement New feature or request labels Nov 15, 2023
@jamietanna jamietanna merged commit a64d36d into oapi-codegen:master Nov 15, 2023
8 checks passed
@jamietanna
Copy link
Member

Thank you @renom 👏 cc @itpavelkozlov

@jamietanna jamietanna changed the title Pass .Required to BindStyledParameterWithLocation and BindStyledParameter Pass .Required to BindStyledParameterWithLocation and BindStyledParameter Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error on header value of empty string when it isn't required
2 participants