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

[Scala] added new scala-cask generator for the cask framework #18046

Closed
wants to merge 27 commits into from

Conversation

aaronp
Copy link
Contributor

@aaronp aaronp commented Mar 6, 2024

This change adds a new scala-cask generator for the cask framework.

Thanks in advance to scala committee reviewers @clasnake, @jimschubert, @shijinkui, @ramzimaalej, @chameleon82, @Bouillie and/or @Fish86

PR checklist

  • [ x] Read the contribution guidelines.
  • [ x] 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.1.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.

.gitignore Outdated
@@ -282,3 +282,4 @@ samples/openapi3/client/petstore/go/privatekey.pem

## OCaml
samples/client/petstore/ocaml/_build/
samples
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR. Why add this 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.

Ah - I think that's a hang over from what I was using as an output directory. I'll remove it. Thanks

<option name="Make" enabled="true" />
</method>
</configuration>
</component>
Copy link
Member

Choose a reason for hiding this comment

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

what's the purpose of this 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.

It was a run profile for testing -- something I can just take out

@wing328
Copy link
Member

wing328 commented Mar 9, 2024

thanks for the PR.

can you please also add the new sample folder to .github/workflows/samples-scala.yaml so that the CI can test it moving forward?

@wing328
Copy link
Member

wing328 commented Mar 13, 2024

please ping me via Slack if you need help with this PR.

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

thanks again for the PR

@aaronp
Copy link
Contributor Author

aaronp commented Mar 21, 2024

I think I've addressed the comments in the PR - many thanks @wing328 for the review!

@aaronp
Copy link
Contributor Author

aaronp commented Mar 22, 2024

How very exciting! Thanks all for your time and patience in reviewing -- very much appreciated!

@aaronp
Copy link
Contributor Author

aaronp commented Mar 26, 2024

Thanks @wing328 for the review and help. I've just updated the branch and included server validation improvements, so hopefully is ready for another review?

@wing328
Copy link
Member

wing328 commented Mar 27, 2024

can you please take a look at the CI build failure when you've time?

https://github.com/OpenAPITools/openapi-generator/actions/runs/8440910756/job/23134572125?pr=18046

let me know if you need help to fix it.

@wing328
Copy link
Member

wing328 commented Mar 27, 2024

the scala cask tests also failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/8440910764/job/23134573837?pr=18046

please take a look when you've time

@wing328
Copy link
Member

wing328 commented Mar 28, 2024

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Member

wing328 commented Mar 30, 2024

the test failed. can you please take a look?

https://github.com/OpenAPITools/openapi-generator/actions/runs/8468026875/job/23262734520?pr=18046

@aaronp
Copy link
Contributor Author

aaronp commented Mar 30, 2024 via email

@aaronp
Copy link
Contributor Author

aaronp commented Mar 30, 2024 via email

@wing328
Copy link
Member

wing328 commented Mar 30, 2024

can you review and fix: https://github.com/OpenAPITools/openapi-generator/actions/runs/8492216876/job/23265283400?pr=18046?

for test failures with other scala samles, please ignore those for the time being

public OperationGroup(String httpMethod, String pathPrefix) {
this.httpMethod = httpMethod;
this.pathPrefix = pathPrefix;
caskAnnotation = "@cask." + httpMethod.toLowerCase();
Copy link
Member

Choose a reason for hiding this comment

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

for toLowerCase, you will need to use Locale.ROOT to address the build failure:

additionalProperties.put("httpClientGprName", httpClientPackageName.toLowerCase(Locale.ROOT));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Boom! Smashing through these. Fixed - thanks

Copy link
Member

Choose a reason for hiding this comment

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

can you please also fix the rest?

Forbidden method invocation: java.lang.String#toUpperCase() [Uses default locale]
Error:    in org.openapitools.codegen.languages.ScalaCaskServerCodegen (ScalaCaskServerCodegen.java:376)
Error:  Forbidden method invocation: java.lang.String#toUpperCase() [Uses default locale]
Error:    in org.openapitools.codegen.languages.ScalaCaskServerCodegen (ScalaCaskServerCodegen.java:37[9](https://github.com/OpenAPITools/openapi-generator/actions/runs/8505836811/job/23295001534?pr=18046#step:5:10))
Error:  Scanned 529 class file(s) for forbidden API invocations (in 1.00s), 2 error(s).
Error:  Failed to execute goal de.thetaphi:forbiddenapis:3.5.1:check (default) on project openapi-generator: Check for forbidden API calls failed, see log. -> [Help 1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yepper - done

@wing328
Copy link
Member

wing328 commented Apr 1, 2024

can you please take a look at https://github.com/OpenAPITools/openapi-generator/actions/runs/8505982227/job/23295571166?pr=18046 ?

@wing328
Copy link
Member

wing328 commented Apr 1, 2024

samples seems not up-to-date: https://github.com/OpenAPITools/openapi-generator/actions/runs/8505982218/job/23295650385?pr=18046

please regenerate the samples

array = pathPrefix.split(File.separator, -1);
} catch (PatternSyntaxException exp) {
throw new RuntimeException("For some reason, splitting >" + pathPrefix + "< on >" + File.separator + "< threw " + exp, exp);
}
Copy link
Member

Choose a reason for hiding this comment

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

looks like this line is throwing exception in windows?

what about using just / instead of File.separator, which is \ in windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch - done

protected String gitUserId = env("GIT_USER", System.getProperty("user.name"));
protected String gitRepoId = env("GIT_REPO", artifactId);
protected String appName = env("APP_NAME", "Cask App");
protected String infoEmail = env("INFO_EMAIL", "[email protected]");
Copy link
Member

Choose a reason for hiding this comment

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

for gitUserId and gitRepoId, you can use these directly from default codegen.

please do not use environment variables, and use additional properties instead. e.g. https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaAkkaHttpServerCodegen.java#L174-L224

infoEmail should be obtained from the spec so I don't think we need to set it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks.

I've reworked these properties and cleaned up a lot of the debug code.

I've changed the sample properties and output destination -- I hope that's not too confusing to follow from the original PR

@aaronp
Copy link
Contributor Author

aaronp commented Apr 5, 2024

@wing328, it looks like this might be good to go, unless I've missed something?

@@ -32,6 +32,7 @@ jobs:
- samples/server/petstore/scalatra
- samples/server/petstore/scala-finch # cannot be tested with jdk11
- samples/server/petstore/scala-http4s-server
- samples/server/petstore/scala/cask
Copy link
Member

Choose a reason for hiding this comment

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

should it be samples/server/petstore/scala-cask instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes - good catch. updated

@@ -0,0 +1,53 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

the "mo/del" folder looks a bit weird to me.
should it be simply just "model"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just testing out weird/nested configs, but you're right. I've updated it now to be a more standard package structure

@wing328
Copy link
Member

wing328 commented Apr 10, 2024

i've filed #18344 based on this PR with some minor enhancements.

@wing328
Copy link
Member

wing328 commented Apr 10, 2024

merged #18344 (commits still authored by you).

thanks for the new generator 👍

@wing328 wing328 closed this Apr 10, 2024
@aaronp aaronp deleted the cask branch April 24, 2024 10:29
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.

None yet

2 participants