Skip to content

New tapiro imports #373

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

New tapiro imports #373

wants to merge 5 commits into from

Conversation

lvolp
Copy link
Contributor

@lvolp lvolp commented Apr 26, 2021

The idea is to generate imports from controller imports instead of model packages.
If is not possible to find a specific import for a type in a root, all the “._” imports present in the route paths will be included .
This will lead to some unused imports (that can be removed with Scalafix), but adds support to types not present in the model (such UUID,LocalDateTime, etc)

@kanbanbot kanbanbot added the WIP label Apr 26, 2021
@calippo calippo requested a review from tpetrucciani April 26, 2021 10:26
@kanbanbot kanbanbot added in review and removed WIP labels Apr 26, 2021
@calippo
Copy link
Member

calippo commented Apr 26, 2021

@lvolp testoni

@lvolp
Copy link
Contributor Author

lvolp commented Apr 26, 2021

@lvolp testoni

Ecco cosa mi ero dimenticato 🤣

@tpetrucciani
Copy link
Contributor

I've tried it on our project, and it works for me, with a caveat: at first I got compile errors using the newly generated routes. This was because it was not only adding the controller imports, but also imports from other files (because tapiroRoutesPaths was set to the whole folder of the api module, that includes Boot.scala too, for example). So I got an extra import cats.syntax.reducible._ in addition to cats.implicits._ and they were conflicting.

Once I fixed tapiroRoutesPaths to a folder with only the controllers, it worked.


I have a doubt on this approach when relative imports are used:

  • In the generated endpoints, I have some relative imports such as import model.X where model is actually myProject.apiGateway.model. These have been copied from the controller.
  • These imports work because the endpoints are generated in the same package as the controllers.
  • However, if the endpoints were generated in a different package, I suppose these endpoints wouldn't work.

I'm not sure whether this is a serious problem though, so this is probably still OK for now and we'll deal with it if it arises. In practice, I suppose we normally use the same package (or sub-packages such that relative imports still work).

@lvolp
Copy link
Contributor Author

lvolp commented Apr 30, 2021

I have a doubt on this approach when relative imports are used:

  • In the generated endpoints, I have some relative imports such as import model.X where model is actually myProject.apiGateway.model. These have been copied from the controller.
  • These imports work because the endpoints are generated in the same package as the controllers.
  • However, if the endpoints were generated in a different package, I suppose these endpoints wouldn't work.

I'm not sure whether this is a serious problem though, so this is probably still OK for now and we'll deal with it if it arises. In practice, I suppose we normally use the same package (or sub-packages such that relative imports still work).

You're right, I didn’t considered relative imports.
I’m also working on a pr, to run scalafix on generated files.
Scalafix should be able to expand relative imports using a semantic rule.
If relative imports are used in buildo projects, probably the best thing to do is pause this pr and wait for scalafix.

Copy link
Contributor

@tpetrucciani tpetrucciani left a comment

Choose a reason for hiding this comment

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

A few comments on legibility. Maybe some of the complexity can be removed by just generating more imports, even if unneeded, and then eliminating them via scalafix?

Also, some files are not scalafmt-formatted. However, I noticed this is not checked in the CI (and also unrelated files are not formatted properly): I've opened a separate issue #377, so we can fix all formatting in that issue.

@@ -8,6 +8,29 @@ import scala.meta.contrib._
import cats.data.NonEmptyList

object Meta {

val controllerImports =(routes: List[TapiroRoute],imports: Set[Importer],outputPackage:String) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is a bit hard to read. I'll suggest a few small changes below, but also: how much of the complexity is done to avoid redundant imports? Since we're getting redundant imports anyway, maybe we can simplify a bit even if we get more of them? (and then either remove them with scalafix or suppress them during compilation)

@@ -8,6 +8,29 @@ import scala.meta.contrib._
import cats.data.NonEmptyList

object Meta {

val controllerImports =(routes: List[TapiroRoute],imports: Set[Importer],outputPackage:String) => {
val types = routes.flatMap(tr => tr.route.params.map(r=> r.tpe) ++ (if (tr.route.error.isDefined) List(tr.route.error.get,tr.route.returns) else List(tr.route.returns)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val types = routes.flatMap(tr => tr.route.params.map(r=> r.tpe) ++ (if (tr.route.error.isDefined) List(tr.route.error.get,tr.route.returns) else List(tr.route.returns)))
val types = routes.flatMap(tr => tr.route.params.map(_.tpe) ++ tr.route.error.toList ++ List(tr.route.returns)))

should be equivalent I think?


val controllerImports =(routes: List[TapiroRoute],imports: Set[Importer],outputPackage:String) => {
val types = routes.flatMap(tr => tr.route.params.map(r=> r.tpe) ++ (if (tr.route.error.isDefined) List(tr.route.error.get,tr.route.returns) else List(tr.route.returns)))
val typeNameList = types.flatMap(typeNameExtractor(_)).toSet
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
val typeNameList = types.flatMap(typeNameExtractor(_)).toSet
val typeNameList = types.flatMap(typeNameExtractor).toSet

val types = routes.flatMap(tr => tr.route.params.map(r=> r.tpe) ++ (if (tr.route.error.isDefined) List(tr.route.error.get,tr.route.returns) else List(tr.route.returns)))
val typeNameList = types.flatMap(typeNameExtractor(_)).toSet
val (wildcardImportList,importList) = imports.collect{
case i : Importer if i.syntax.endsWith("._") => List(i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could extract a function isWildcardImport instead of repeating this check here and on line 18?

case Importer(ref,importees) => importees.filter(p=> typeNameList.contains(p.syntax)).map(p=> Importer(ref,List(p)))
}.flatten.partition(_.syntax.endsWith("._"))
val controllerPackageStrings = routes.map(r=> s"import ${r.route.controllerPackage.mkString(".")}._")
val controllerPackage : List[Import] =controllerPackageStrings.flatMap(_.parse[Source].getOrElse(Source(List())).tree.children).collect{case i: Import => i}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit strange since we're first building the strings and then constructing the imports by parsing the strings. I think it would be nicer to build the imports directly using quasiquotes (and maybe extract the strings from them). But I'm not sure if this is too cumbersome, otherwise it's fine to leave as is.

val importers = (if (importList.flatMap(i=> typeNameList.diff(i.importees.map(_.syntax).toSet)).isEmpty) importList else {
(wildcardImportList ++ importList)
}).map(i=>Import(List(i)))
val result= if (controllerPackageStrings.filter(_.endsWith(outputPackage+"._")).isEmpty) controllerPackage ++ importers else importers
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use forall or exists instead of filter/isEmpty?

Also, I'm not sure I've understood what the condition in the if means, maybe we can rewrite to

val someMeaningfulNameForTheCondition = controllerPackageStrings.filter(_.endsWith(outputPackage+"._")
val result= if (someMeaningfulNameForTheCondition) controllerPackage ++ importers else importers

using some meaningful name to make this more understandable.

}).map(i=>Import(List(i)))
val result= if (controllerPackageStrings.filter(_.endsWith(outputPackage+"._")).isEmpty) controllerPackage ++ importers else importers
deduplicate(result.toList).toSet
} : Set[Import]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the type annotation to after the arguments

@tpetrucciani
Copy link
Contributor

If relative imports are used in buildo projects, probably the best thing to do is pause this pr and wait for scalafix.

They're not used much, and anyway our endpoints are generated in the same package as the controller, so it's not an issue for now (though it would be best to support them in the future).

But it could make sense to add scalafix first if it also allows us to simplify this code by not caring about generating too many imports.

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.

4 participants