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

controllers package should be under play package #6555

Open
gmethvin opened this issue Sep 14, 2016 · 7 comments
Open

controllers package should be under play package #6555

gmethvin opened this issue Sep 14, 2016 · 7 comments

Comments

@gmethvin
Copy link
Member

I've never liked the idea of having a top-level package called controllers for controllers that are part of the framework like Assets and Default. Since they are Play-provided components, they should be within the play package. I think play.controllers would be reasonable. This also means our built-in controllers can use things that are private[play].

I think the top-level controllers makes the routes less clear when used in conjunction with other user-provided controllers in the controllers package (as opposed to namespacing to an organization like com.typesafe.controllers). It should be obvious which controllers are defined by your code and which are not.

@wsargent
Copy link
Member

Agreed. I'd even go further and say Assets and Default should be in a different subproject from play, so they can be excluded as library dependencies for projects that want their own behavior.

@gmethvin
Copy link
Member Author

@wsargent Yeah, that makes sense, though I think we should still include the dependency by default in the SBT plugin.

@wsargent
Copy link
Member

Agreed.

@gmethvin gmethvin added this to the 2.6.0 milestone Sep 15, 2016
@gmethvin
Copy link
Member Author

Marking this for 2.6 since it seems like a simple migration, but we can always push it back to 3.0 if needed.

@balagez
Copy link

balagez commented Jul 10, 2017

Having to do this when I need most things from play.api when wiring the application using compile time DI is just lame:

import play.api.{ controllers => _, _ }
import controllers.AssetsComponents

@jxtps
Copy link
Contributor

jxtps commented Jul 14, 2017

To expand on @balagez comment as I also ran into this issue: IntelliJ likes to shorten multiple play.api.Xyz imports to play.api._. This then makes controllers.Xyz refer to play.api.controllers.Xyz which doesn't exist.

Currently the only thing in the play.api.controllers package is TrampolineContextProvider.

Since it's typical to import so much from play._ and play.api._ it might make sense to not re-use the exact same name (controllers) as it leads to this type of shadowing.

@marcospereira marcospereira modified the milestones: 2.6.2, 2.6.3 Jul 20, 2017
@gmethvin gmethvin modified the milestones: 2.7.0, 2.6.3 Aug 7, 2017
@gmethvin
Copy link
Member Author

gmethvin commented Aug 7, 2017

The main issue here is the way the reverse router works now. Let's say you have a routes file in the com.example package. The way it works now, the routes compiler will generate a com.example.play.controllers package if you reference the play.controllers package in your routes file. That will create a conflict anytime you need to reference the play package within the com.example package.

Maybe we should separate the pieces like AssetsComponents and other such classes into a play.assets package and keep the Assets controller in the root controllers package. Or perhaps we need to change the way the routes generator creates classes.

This requires a lot more thought than I thought it did originally, which is why it didn't make it into 2.6. If we can find a nice backwards compatible solution it could make it into 2.6.x sometime but it's not something we're working on right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants