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

[#2185] Remove useless warning in reverse compiler #2197

Merged

Conversation

baloo
Copy link
Contributor

@baloo baloo commented Dec 23, 2013

see #2185

GET     /dummy       controllers.Application.dummy()
GET     /dummy1      controllers.Application.dummy()

will not generate warning

GET     /dummy       controllers.Application.dummy(foo: String)
GET     /dummy1      controllers.Application.dummy(foo = "bar")

will still generate warning

GET     /dummy1      controllers.Application.dummy(foo = "bar")
GET     /dummy       controllers.Application.dummy(foo: String)

will not generate warning

genCall(route, localNames))
// Routes like /dummy controllers.Application.dummy(foo = "bar")
// foo = "bar" is a constraint
val parametersContraints = Option(route.call.parameters.getOrElse(Nil).filter { p =>
Copy link
Member

Choose a reason for hiding this comment

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

I think we can rewrite this a bit to make it more clear:

val parametersConstraints = (route.call.parameters.getOrElse(Nil).collect {
  case p if localNames.contains(p.name) && p.fixed.isDefined => s"${p.name} == ${p.fixed.get}"
}) match {
  case Seq() => ""
  case nonEmpty => s"if ${nonEmpty.mkString(" && ")}"
}

@gmethvin
Copy link
Member

Can we add a test for this in RoutesCompilerSpec?

@baloo
Copy link
Contributor Author

baloo commented Dec 24, 2013

I'm sorry but I'm unable to test this in RoutesCompilerSpec, the warning occurs during the second phase of compilation (converting scala code into bytecode using scalac). I do not know the scala compiler internals, I have no idea how to test the output of scalac :(

```
GET     /dummy       controllers.Application.dummy()
GET     /dummy1      controllers.Application.dummy()
```
will not generate warning

```
GET     /dummy       controllers.Application.dummy(foo: String)
GET     /dummy1      controllers.Application.dummy(foo = "bar")
```
will still generate warning

```
GET     /dummy1      controllers.Application.dummy(foo = "bar")
GET     /dummy       controllers.Application.dummy(foo: String)
```
will not generate warning
@jroper
Copy link
Member

jroper commented Jan 2, 2014

Nice.

It would be possible to implement such a test, we have code that invokes the Scala compiler in the templates compiler specs, but it is a lot of overhead and quite annoying to maintain across different Scala versions because that API changes all the time, so I won't make you port that code to the routes compiler.

jroper added a commit that referenced this pull request Jan 2, 2014
…arnings

[#2185] Remove useless warning in reverse compiler
@jroper jroper merged commit 695d88f into playframework:master Jan 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants