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

Remove appendRequestToHandler #19500

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Remove appendRequestToHandler #19500

merged 1 commit into from
Sep 4, 2024

Conversation

Rugal
Copy link
Contributor

@Rugal Rugal commented Aug 31, 2024

The original thought was to make generator able to add ServerHttpRequest, but turns out using templating way to do customization is much more easier than adding parameter, plus my previous change is not so well and not very useful

The original 2 PR are listed here, I would like to revert all my changes

  1. Able to generate within parameter #17158 #17623
  2. Add ability to append ServerHttpRequest for kotlin-spring generator #17158

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@wing328 @jimschubert

@wing328
Copy link
Member

wing328 commented Sep 2, 2024

cc
@dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03) @stefankoppier (2022/06)

@wing328
Copy link
Member

wing328 commented Sep 4, 2024

no problem @Rugal

thanks for the contributions.

look forward to more contributions from you.

@wing328 wing328 merged commit f54b0e2 into OpenAPITools:master Sep 4, 2024
32 checks passed
@querwurzel
Copy link

querwurzel commented Oct 25, 2024

@Rugal Can you elaborate on why exactly this isn't useful and what better alternative there is?
I would even argue that the response object is also required in some cases.

Background: This change was released with 7.9.0 and now obviously breaks builds.

@Rugal
Copy link
Contributor Author

Rugal commented Oct 25, 2024

@Rugal Can you elaborate on why exactly this isn't useful and what better alternative there is? I would even argue that the response object is also required in some cases.

Background: This change was released with 7.9.0 and now obviously breaks builds.

Response object is not enough, we need the exchange object in order to get both request and response information. To do that, simply updating the template in your configuration should work already, no need to have extra option to generate that response object solely.

Please refer to the official site for templating related stuffs. It is super easy to understand and use.

@querwurzel
Copy link

querwurzel commented Oct 25, 2024

@Rugal Can you elaborate on why exactly this isn't useful and what better alternative there is? I would even argue that the response object is also required in some cases.
Background: This change was released with 7.9.0 and now obviously breaks builds.

Response object is not enough, we need the exchange object in order to get both request and response information. To do that, simply updating the template in your configuration should work already, no need to have extra option to generate that response object solely.

Please refer to the official site for templating related stuffs. It is super easy to understand and use.

Thank you for the explanation, I will take a look into the templating section.
Im aware the exchange object refers to both req/resp and I'd like to have an option that injects it into all controller methods.

Shipping this breaking change with a minor version update is against all expectations, be it OSS or not.

@wing328 @jimschubert

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

Successfully merging this pull request may close these issues.

3 participants