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

Import cleanup #16368

Merged
merged 2 commits into from
Apr 6, 2017
Merged

Import cleanup #16368

merged 2 commits into from
Apr 6, 2017

Conversation

TBonnin
Copy link
Contributor

@TBonnin TBonnin commented Apr 5, 2017

What does this change?

This patch:

  • removes some unused imports
  • removes template auto import using TwirlKeys.templateImports setting: It makes templates dependencies hard to follow. Instead importing the necessary dependencies in each templates.

Example: football templates were automatically importing pa._ which provides an implicit StringToOption function. It was not clear to me why passing a String to a fragment that was expecting an Option[String] argument was compiling successfully

I wanted to enable the -Ywarn-unused-import compiler option but it doesn't play well with Twirl templates: playframework/twirl#105

What is the value of this and can you measure success?

More explicit dependencies, less hidden magic

Tested in CODE?

yes

@PRBuilds
Copy link

PRBuilds commented Apr 5, 2017

PR build results:

screenshots
mobile.pngwide.pngtablet.pngdesktop.png

exceptions (0)
thrown-exceptions.js

webpagetest (2)
performanceComparisonSummary.txt

-automated message

@@ -88,17 +87,10 @@ object Frontend extends Build with Prototypes {
libraryDependencies ++= Seq(
paClient,
akkaContrib
),
TwirlKeys.templateImports ++= Seq(
Copy link
Contributor

Choose a reason for hiding this comment

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

😰

@@ -3,8 +3,7 @@
@import implicits.Football._
@import views.FootballHelpers._


@(page: MatchPage, competition: Option[model.Competition])(implicit request: RequestHeader, context: model.ApplicationContext)
@(page: _root_.football.controllers.MatchPage, competition: Option[model.Competition])(implicit request: RequestHeader, context: model.ApplicationContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why these have to be _root_, is there another MatchPage it might pick up otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. It is not necessary.
However IntelliJ is confused and pick up the football package from another app (admin). That's why I originally added _root_.
I removed it and will try to fix IntelliJ instead

Copy link
Contributor

Choose a reason for hiding this comment

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

will try to fix IntelliJ instead

Good luck, you may need it! 😄

Copy link
Contributor

@jfsoul jfsoul left a comment

Choose a reason for hiding this comment

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

Lgtm, just had one question

Copy link
Contributor

@NataliaLKB NataliaLKB left a comment

Choose a reason for hiding this comment

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

Yay to being more explicit 🎉 !

It makes templates dependencies hard to follow.
Instead importing the necessary dependencies in each templates
@TBonnin TBonnin force-pushed the tb-unused-imports branch from 6c91e24 to 3a3a4fb Compare April 5, 2017 16:27
@TBonnin TBonnin merged commit 7136889 into master Apr 6, 2017
@TBonnin TBonnin deleted the tb-unused-imports branch April 6, 2017 09:24
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @TBonnin 13 minutes and 46 seconds ago) Please check your changes!

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.

5 participants