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.

Sign up for GitHub

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

Jump to bottom

feat(avro)/refactor(core): unify *_POST_PROCESS_FILE behaviour and reuse code #19761

Merged
merged 5 commits into from
Oct 5, 2024
Merged

feat(avro)/refactor(core): unify *_POST_PROCESS_FILE behaviour and reuse code #19761

merged 5 commits into from
Oct 5, 2024

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Oct 2, 2024 •
edited
Loading

This PR does three things:

  • It adds support for a new AVRO_POST_PROCESS_FILE env var that is respected when file post processing is enabled
  • It refactors duplicated code from 20+ generators to be moved up into the DefaultCodeGen to be reused
  • It adds a warning to each of the generators that haven't had it yet, when file post processing is enabled but no <FILETYPE>_POST_PROCESS_FILE flag is passed

@wing328 Given that this PR touches so many generators but only refactors, doesn't change main behaviour, do I need reviews from each generator ownership team?

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.x.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.

joscha
joscha commented Oct 2, 2024
View reviewed changes
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java Outdated
@@ -8088,6 +8092,32 @@ public void postProcessFile(File file, String fileType) {
LOGGER.debug("Post processing file {} ({})", file, fileType);
}

protected void executePostProcessor(String[] commandArr) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were two different variants of this code. The majority without showing the error output in case the execution was non-zero. I unified them here, taking error output into account.

joscha
joscha commented Oct 2, 2024
View reviewed changes
...openapi-generator/src/main/java/org/openapitools/codegen/languages/CrystalClientCodegen.java
@@ -890,6 +888,7 @@ public String escapeUnsafeCharacters(String input) {

@Override
public void postProcessFile(File file, String fileType) {
super.postProcessFile(file, fileType);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a few were missing super calls. The super class only has a logger statement in it currently.

joscha
joscha commented Oct 2, 2024
View reviewed changes
...openapi-generator/src/main/java/org/openapitools/codegen/languages/DartDioClientCodegen.java
@@ -155,11 +154,6 @@ public String getHelp() {
public void processOpts() {
super.processOpts();

if (StringUtils.isEmpty(System.getenv("DART_POST_PROCESS_FILE"))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already available in AbstractDartCodegen which this class inherits from.

joscha
joscha commented Oct 2, 2024
View reviewed changes
.../openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java
@@ -178,11 +178,6 @@ public void processOpts() {
// map to Dot instead of Period
specialCharReplacements.put(".", "Dot");

if (StringUtils.isEmpty(System.getenv("PYTHON_POST_PROCESS_FILE"))) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already available in AbstractPythonCodegen which this class inherits from.

joscha
joscha commented Oct 2, 2024
View reviewed changes
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
Comment on lines +8102 to +8108
try (InputStreamReader inputStreamReader = new InputStreamReader(p.getErrorStream(), StandardCharsets.UTF_8);
BufferedReader br = new BufferedReader(inputStreamReader)) {
StringBuilder sb = new StringBuilder();
String line;
while ((line = br.readLine()) != null) {
sb.append(line);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

majority of implementations didn't have this very helpful part

@joscha joscha marked this pull request as ready for review October 2, 2024 22:16
@joscha joscha changed the title feat(avro)/refactor(core): unify *_POST_PROCESS_FILE bahviour and code feat(avro)/refactor(core): unify *_POST_PROCESS_FILE behaviour and reuse code Oct 2, 2024
@wing328
Copy link
Member

wing328 commented Oct 3, 2024

@wing328 Given that this PR touches so many generators but only refactors, doesn't change main behaviour, do I need reviews from each generator ownership team?

let me review first and reach out to them if needed.

thanks for the PR

@wing328 wing328 added Feature: Generator Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc. labels Oct 5, 2024
@wing328 wing328 added this to the 7.9.0 milestone Oct 5, 2024
@wing328
Copy link
Member

wing328 commented Oct 5, 2024

lgtm. thanks for the refactoring

@wing328 wing328 merged commit 899ddec into OpenAPITools:master Oct 5, 2024
15 checks passed
@wing328 wing328 added the Schema: Avro Generation of Avro schemas label Oct 5, 2024
@joscha joscha deleted the joscha/unify-post-process-file branch October 5, 2024 10:44
@joscha
Copy link
Contributor Author

joscha commented Oct 5, 2024

lgtm. thanks for the refactoring

Thanks for merging. There's a generator scaffold somewhere, should we update it there too or is it quite individual for new generators to get a post processor?

@wing328
Copy link
Member

wing328 commented Oct 5, 2024

There's a generator scaffold somewhere,

feel free to update it. thanks

(I wasn't the one creating it. To create a new generator, I usually ask users to clone an existing generator to start with)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc. Feature: Generator Schema: Avro Generation of Avro schemas
Projects
None yet
Milestone
7.9.0
Development

Successfully merging this pull request may close these issues.
2 participants
@joscha @wing328