Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.1k
Remove StaticRoutesGenerator#8049
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3348ec7 toc8803e9CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggested removing the sbt setting used to choose a different generator, since now there's only one kind. Not sure if that was something you were planning for a future PR or not.
| @@ -127,18 +126,12 @@ object RoutesCompiler extends AutoPlugin with RoutesCompilerCompat { | |||
| }.value, | |||
| namespaceReverseRouter := false, | |||
| routesGenerator := InjectedRoutesGenerator, // changed from StaticRoutesGenerator in 2.5.0 | |||
| routesGenerator := InjectedRoutesGenerator, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we get rid of theroutesGenerator setting entirely now that there's only one type of generator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was wondering that too, but thinking it doesn't hurt to leave it there just in case someone wants to configure a custom routes generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That sounds fair enough. I guess I'm always a fan of reducing API surface if we can. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
well I think we should let it there. since sird is better and better, people can actually create aNoOpRoutesGenerator, which can be used with sird. This will save a lot of unnecessary IO while looking for route files. (and there are various other way's why people would create their own..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@schmitch wouldn't it be better to have Play provide aNoOpRoutesGenerator for people in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
well I'm not sure how many people would actually need it and it's not really that hard to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or they could just remove theRoutesCompiler plugin. We have aPlayService plugin in 2.7 that is more minimal and does not include the routes compiler.
I have no problem with leaving the option in, though. The main point of this PR is to avoid having to maintain the static routes compiler. I could also see value in allowing someone to provide a routes generator that uses a completely different mechanism to generate a router or customizes the default generator in some way. It's possible people are already doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm OK with leaving the option in. FYI I just changed my review to 👍 since I'm not requesting changes anymore.
Ok sounds good to me! |
Removes the static routes generator, references the removal in the the migration guide, and removes references to the static routes generator in the documentation.
Fixes#8047.