Remove package objects from models and controllers #489
Conversation
These files are used for documentation only. Useful, but prohibit me from adding package objects with real logic in my project.
|
@rschellhorn That's not entirely true. The empty package objects make sure that the default template imports won't fail. |
|
If I understand correctly these files are solely there to make sure these packages exist and the import would not complain about non-existing symbols, right? Is there any other workaround for that, e.g. another placeholder file? Package objects can be useful for clients. For instance I'd like to be able to put my implicit conversions there (e.g. model to Json writes). |
|
@rschellhorn when it comes to client code, storing implicit conversions in package object for convenience is usually not the best strategy. In most cases you are probably much better off by defining and mixing in implicits via traits (so you can limit the scope as much as possible) |
|
@pk11 in most cases I have to agree. However there are are a few cases where placing stuff in a package object would be very useful. I guess that is the reason why these constructs were introduced in the Scala language in the first place. Maybe json serialization is a poor example. A more convincing one: In my current app I store the authenticated user on the request and this needs to be translated to some sort of Authority while invoking service methods (for authoring, logging, etc). This authority class and it's conversion is something I want to do implicitly, because it is mostly boilerplate. The (implicit) conversion is something I'd like to put in a package object. What I'm trying to prevent is importing a certain object (or mixin a trait) in every single controller class. |
|
@rschellhorn what's wrong with (the benefit of this approach is that implicits would be applied only on real controller classes, not everything that you may define in the controllers package, furthermore, you are also more explicit about your implicit usage which is usually a good thing IMHO) |
|
@pk11 There is nothing wrong with that. Imho Scala has three good candidate locations to put such logic in, each with benefits and drawbacks. A trait, an object (+ import) or a package object. The choice is a matter of scope and personal preference I guess... But maybe the discussion should be that Play now takes away one of those options for all client applications, solely because of an implementation detail of the framework. If there is a workaround for SBT not removing empty packages (that is what we are talking about, right?) then I think that is a better option. I'll dive into the SBT options and see what's possible there. |
|
@rschellhorn the problem is removing that package object just to support your specific use case would potentially be a breaking change for other users. I can see that we'd address this problem in 2.2 but I feel like we already have enough breaking changes in 2.1 and this one is just not worth it IMHO. (There are workarounds, too: use a different package, use traits etc.) Anyway, I'll ask other core devs to chime in and see what they think |
|
@rschellhorn So please at least create dummy objects in both packages to keep things compiling. |
These files are used for documentation only. Useful, but prohibit me from adding package objects with real logic in my project.
|
Hi @pk11, @guillaumebort, I just pushed a fix that will keep empty projects (no models and no controllers, but with template files) compiling. The fix contains a placeholder file in the models package. It seems no placeholder is necessary for controllers. |
|
@rschellhorn yes, |
|
Done and signed the CLA as well. |
|
What's the status on this? Has it been approved for merging in 2.1? It looks similar to an issue that was reported over a year ago which got dismissed (https://play.lighthouseapp.com/projects/82401/tickets/55-package-object-controllers). If this pull request is going in the same direction, I'd like to weigh in on the side of rschellhorn. It would be very nice to be able to define type aliases in the models package object. For my particular use case, I have fairly big application with various components and a set of common models defined in a separate package. It would make a lot of sense for me to simply add a few type aliases for the subset of models I need in the Play! application and have access to them in the templates without having import statements or complete paths all over the place. It might just look to you like a tiny detail, but when you have 40+ templates and you pass around the models as arguments to these templates, you begin to feel the pain. I understand that there are many ways of working around this issue in the general case, but I must say that it feels "wrong" for the Play! framework to restrain developers from creating a package object which would be perfectly valid in Scala and has no real reason for being disallowed (for God's sake, it's currently an empty package whose sole purpose is to be a placeholder!). At the very least, there should be a warning at compile time that this particular package object is not allowed in Play! apps or some warning against using it in the documentation. I also understand that there are many breaking changes in 2.1, but I fail to see how the proposed change would ever break any client code. Making the framework less restrictive could only make it more powerful. |
|
I will check with the other play team members |
Remove package objects from models and controllers
5 participants
These files are used for documentation only. Useful, but prohibit me
from adding package objects with real logic in my project.