[LagomScala] Add Scala API generator for lagomframework #6900 by gmkumar2005 · Pull Request #6901 · swagger-api/swagger-codegen

gmkumar2005 · 2017-11-07T13:49:34Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: 2.3.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

This change will support code generation of Lagom Scala API. Lagom Scala Api project can be used for writing an implementation or as a client to consume an external service.
####Features which are not supported

Form Parameters
Seq in query parameters
File Handling

wing328 · 2017-11-08T07:39:04Z

...es/swagger-codegen/src/main/java/io/swagger/codegen/languages/AbstractScalaLagomCodegen.java

+import java.io.File;
+import java.util.*;
+
+public abstract class AbstractScalaLagomCodegen extends DefaultCodegen {


@gmkumar2005 Instead of creating another abstract class, what about using AbstractScalaCodegen.java instead?

wing328 · 2017-11-08T07:43:02Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/LagomScalaApiCodegen.java

+
+import java.util.*;
+
+public class LagomScalaApiCodegen extends AbstractScalaLagomCodegen implements CodegenConfig {


I would suggest naming it as "ScalaLagomServerCodegen" following the naming convention of other server generator.

wing328 · 2017-11-08T07:43:42Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/LagomScalaApiCodegen.java

+
+  @Override
+  public CodegenType getTag() {
+    return CodegenType.CLIENT;


Please use CodegenType.SERVER instead.

wing328 · 2017-11-08T07:45:17Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/LagomScalaApiCodegen.java

+
+  @Override
+  public String getName() {
+    return "lagomScalaApi";


I would suggest scala-lagom to follow the naming convention "{language}-{lagom}"

Changed to scala-lagomApi to be able to distinguish from impl.

I don't understand that part about distinguish from impl, can you clarify?

wing328 · 2017-11-08T07:46:40Z

modules/swagger-codegen/src/main/resources/META-INF/services/io.swagger.codegen.CodegenConfig

 io.swagger.codegen.languages.TypeScriptNodeClientCodegen
 io.swagger.codegen.languages.UndertowCodegen
 io.swagger.codegen.languages.ZendExpressivePathHandlerServerCodegen
+io.swagger.codegen.languages.LagomScalaApiCodegen


Better to put it according to alpabetical order

wing328 · 2017-11-08T07:51:55Z

modules/swagger-codegen/src/main/resources/lagomScalaApi/plugins.sbt.mustache

@@ -0,0 +1,10 @@
+//
+// Copyright (C) 2016 Lightbend Inc. <https://www.lightbend.com>


Can we remove it? The autogenerated files are unlicensed by default.

wing328 · 2017-11-08T07:53:48Z

We can have another PR later to improve the code format.

Ref: Scala style guide - http://docs.scala-lang.org/style/

wing328 · 2017-11-08T08:14:03Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/LagomScalaApiCodegen.java

+
+  @Override
+  public String getHelp() {
+    return "Generates a Lagom API in scala";


FYI. I'll add the word "beta" later to help manage user expectation similar to what we've done for other new generators.

…ved licensing info from generated code

jimschubert

I won't have time to evaluate in depth for a few days.

At first glance, I only had one comment.

Although, I'm not familiar with Lagom… and the method signatures missing input parameters was weird to me. I'll have to run locally and see if the structure is just how Lagom does things. When I looked at it (I think pre-1.0 when it was only Java), it had a lot of abstractions even in Java.

jimschubert · 2017-11-13T05:16:55Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/ScalaLagomServerCodegen.java

+  public String toVarName(String name) {
+    // sanitize name
+    name = sanitizeName(
+        name); // FIXME: a parameter should not be assigned. Also declare the methods parameters as 'final'.


This looks like a copy/paste. Can it be updated/fixed here, or does that affect something elsewhere?

String modifiedName = name;

Then, use modifiedName within the method?

Jim, you are right it is a copy paste from ScalaClientCodegen.Java. I guess this is needed for scala variables. Not sure what will break. Hence retained the logic.
With reference to method signatures.
Please have a look at https://github.com/lagom/sbt-lagom-descriptor-generator
The method signature generated by this PR matches with the one generated in above project.
Please let me know if you find any gaps.

wing328 · 2017-11-14T13:03:27Z

I ran ./bin/lagomScalaApi-petstore.sh to update the Petstore sample and got the following errors when running sbt test:

[warn] Found version conflict(s) in library dependencies; some are suspected to be binary incompatible:
[warn] 
[warn] 	* com.google.guava:guava:19.0 is selected over 16.0.1
[warn] 	    +- com.lightbend.lagom:lagom-api_2.11:1.3.8           (depends on 16.0.1)
[warn] 	    +- com.google.inject:guice:4.0                        (depends on 16.0.1)
[warn] 
[warn] Run 'evicted' to see detailed eviction warnings
[info] Compiling 9 Scala sources to /private/tmp/swagger-codegen/samples/server/petstore/lagomScalaApi/target/scala-2.11/classes...
[error] /private/tmp/swagger-codegen/samples/server/petstore/lagomScalaApi/src/main/scala/io/swagger/client/api/PetApi.scala:34: type mismatch;
[error]  found   : Seq[String] => com.lightbend.lagom.scaladsl.api.ServiceCall[akka.NotUsed,Seq[io.swagger.client.model.Pet]]
[error]  required: com.lightbend.lagom.scaladsl.api.ServiceSupport.ScalaMethodServiceCall[?,?]
[error]       restCall(Method.GET, "/pet/findByTags?tags", findPetsByTags _), 
[error]                                                    ^
[error] one error found
[error] (compile:compileIncremental) Compilation failed
[error] Total time: 7 s, completed Nov 14, 2017 8:55:33 PM

wing328 · 2017-11-16T13:09:03Z

UPDATE: I did another test and no longer encountered those errors.

wing328 · 2017-11-21T07:44:53Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/ScalaLagomServerCodegen.java

+
+  @Override
+  public String getName() {
+    return "scala-lagomApi";


I'll change this to scala-lagom to conform to our naming convention.

wing328 · 2017-11-21T07:45:13Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/ScalaLagomServerCodegen.java

+
+  @Override
+  public String getHelp() {
+    return "Generates a Lagom API in scala";


I'll add the word "beta" to the description.

wing328 · 2017-11-21T09:56:47Z

@gmkumar2005 I've merged #7011 (enhancements, etc) into master.

Please pull the latest master and let me know if you've any question.

jimschubert · 2017-11-21T10:39:40Z

@wing328 lagom is a framework that supports generating both servers and clients. I see in that PR that you changed the generator to scala-lagom, but I'd recommend either scala-lagom-server or that we provide options for one or the other (or both). The options route would break from the project's standards of separating client and server generators, though. We could generate client/server code without options and require users to lean on ignore files, but that would have to be clearly documented, I'd think.

wing328 · 2017-11-21T10:43:27Z

@jimschubert sure. I'll rename it to scala-lagom-server. Thanks for the feedback.

wing328 · 2017-11-21T15:26:52Z

@jimschubert done via #7014

wing328 · 2017-11-21T16:30:03Z

@gmkumar2005 when you've time, please reach me at my email address listed on my Github profile. Thanks.

Initial Commit

b0145d3

wing328 reviewed Nov 8, 2017

View reviewed changes

wing328 added the Server: Scala label Nov 8, 2017

wing328 added this to the v2.3.0 milestone Nov 8, 2017

wing328 added the Enhancement: New generator label Nov 8, 2017

wing328 reviewed Nov 8, 2017

View reviewed changes

gmkumar2005 added 3 commits November 8, 2017 15:37

Removed unneeded class, Updated names as per naming conventions, Remo…

c34913e

…ved licensing info from generated code

Moved samples from client folder to server

deebf31

Fixed naming issue with scripts in the bin

88d8dbc

jimschubert reviewed Nov 13, 2017

View reviewed changes

improved code format for api

c077fce

gmkumar2005 added 2 commits November 14, 2017 19:57

fixed compile issue and improved formatting

ce4c674

fixed compile issue and improved formatting

eb5f0ec

wing328 merged commit 32abb72 into swagger-api:master Nov 21, 2017

wing328 mentioned this pull request Nov 21, 2017

[Scala][Lagom] enhancement to improve the Lagom server generator #7010

Open

3 tasks

wing328 reviewed Nov 21, 2017

View reviewed changes

wing328 mentioned this pull request Nov 21, 2017

[Scala][Lagom] rename generator from scala-lagom to scala-lagom-server #7014

Merged

4 tasks

wing328 mentioned this pull request Dec 3, 2017

[LagomScala] Add Scala API generator for lagomframework #6900

Closed

gmkumar2005 deleted the issue-6900 branch December 27, 2017 15:34


		import java.util.*;

		public class LagomScalaApiCodegen extends AbstractScalaLagomCodegen implements CodegenConfig {

		@@ -0,0 +1,10 @@
		//
		// Copyright (C) 2016 Lightbend Inc. <https://www.lightbend.com>

Conversation

gmkumar2005 commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimschubert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Nov 14, 2017

Uh oh!

wing328 commented Nov 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Nov 21, 2017

Uh oh!

jimschubert commented Nov 21, 2017

Uh oh!

wing328 commented Nov 21, 2017

Uh oh!

wing328 commented Nov 21, 2017

Uh oh!

wing328 commented Nov 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gmkumar2005 commented Nov 7, 2017 •

edited

Loading