From 1aea118ce27f383f2cf57e77e33f1ecdf81da2b8 Mon Sep 17 00:00:00 2001 From: Roberto Tyley <52038+rtyley@users.noreply.github.com> Date: Tue, 22 Oct 2024 17:19:15 +0100 Subject: [PATCH] Scala 2.13 Scalafix: `ExplicitResultTypes` for implicit definitions This is a precursor to: * https://github.com/guardian/tagmanager/pull/536 Scala 2.13.11 added a warning that implicit definitions [should]( https://nrinaudo.github.io/scala-best-practices/tricky_behaviours/type_implicits.html) have an explicit type (because implicit resolution is already complicated enough for the compiler, and things like file-order can actually make a difference to whether implicit scopes are correctly searched: https://github.com/scala/bug/issues/8697#issuecomment-292432445): * https://github.com/scala/scala/pull/10083 The warning looks like this: ``` [error] ~/code/presence-indicator/app/autoscaling/Notification.scala:7:16: Implicit definition should have explicit type (inferred play.api.libs.json.Reads[autoscaling.Notification]) [quickfixable] [error] implicit val jsonReads = Json.reads[Notification] [error] ^ ``` ...this prepares us for Scala 3, where the explicit type is _required_. More widely, beyond implicit definitions, in **library** code, the best practice is to always add an explicit type to all your **public** members, even when you're happy with what's being inferred - otherwise you can unintentionally break binary-compatibility just by changing the _implementation_ of a field: * https://docs.scala-lang.org/scala3/guides/migration/incompat-type-inference.html * https://nrinaudo.github.io/scala-best-practices/binary_compat/explicit_type_annotations.html * https://scalacenter.github.io/scalafix/docs/rules/ExplicitResultTypes.html ## Automatically fixing this code issue Scalafix actually does a better job than `-quickfix` for this particular task, because it adds imports if it needs to, so that you end up with this in your code: ``` implicit val jsonReads Reads[Notification] = Json.reads[Notification] ``` ...rather than something like: ``` implicit val jsonReads Reads[com.gu.blah.foo.bar.Notification] = Json.reads[Notification] ``` ### Fixing while still on Scala 2.12 - use Scalafix In this commit, we're only trying to fix the implicit definitions, so I've added this in a new `.scalafix.conf` config file: ``` ExplicitResultTypes.onlyImplicits = true ``` The Scalafix rule needs to be run while the project is still on Scala 2.12, not Scala 2.13 (otherwise sbt will say: "Error downloading ch.epfl.scala:sbt-scalafix;sbtVersion=1.0;scalaVersion=2.13:0.13.0"). Once the Scalafix plugin is made available to sbt (by adding `addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.13.0")` to either `project/plugins.sbt` or `~/.sbt/1.0/plugins.sbt`), you can run these commands on the sbt prompt to automatically generate the changes in this PR: ``` scalafixEnable scalafixAll ExplicitResultTypes ``` --- .scalafix.conf | 0 app/model/CampaignInformation.scala | 2 +- app/model/ClientConfig.scala | 3 ++- app/model/ContributorInformation.scala | 2 +- app/model/EditionalisedPage.scala | 3 ++- app/model/ExternalReferenceType.scala | 3 ++- app/model/HyperMedia.scala | 4 ++-- app/model/ImageAsset.scala | 3 ++- app/model/PaidContentInformation.scala | 3 ++- app/model/PublicationInformation.scala | 2 +- app/model/Reference.scala | 2 +- app/model/Sponsorship.scala | 2 +- app/model/Tag.scala | 2 +- app/model/TagSearchResult.scala | 3 ++- app/model/TrackingInformation.scala | 2 +- app/model/command/BatchTagCommand.scala | 3 ++- app/model/command/CreateTagCommand.scala | 5 +++-- app/model/forms/SpreadSheet.scala | 4 ++-- app/model/image.scala | 2 +- app/model/jobs/Step.scala | 2 +- .../sponsorshiplifecycle/SponsorshipLifecycleModule.scala | 2 +- 21 files changed, 31 insertions(+), 23 deletions(-) create mode 100644 .scalafix.conf diff --git a/.scalafix.conf b/.scalafix.conf new file mode 100644 index 00000000..e69de29b diff --git a/app/model/CampaignInformation.scala b/app/model/CampaignInformation.scala index 9d408b82..f86ac271 100644 --- a/app/model/CampaignInformation.scala +++ b/app/model/CampaignInformation.scala @@ -18,7 +18,7 @@ case class CampaignInformation(campaignType: String) { object CampaignInformation { - implicit val trackingFormat = Jsonx.formatCaseClass[CampaignInformation] + implicit val trackingFormat: OFormat[CampaignInformation] = Jsonx.formatCaseClass[CampaignInformation] def apply(thriftCampaignInformation: ThriftCampaignInformation): CampaignInformation = CampaignInformation( diff --git a/app/model/ClientConfig.scala b/app/model/ClientConfig.scala index c224a9dd..2f6e0c6a 100644 --- a/app/model/ClientConfig.scala +++ b/app/model/ClientConfig.scala @@ -2,6 +2,7 @@ package model import ai.x.play.json.Encoders.encoder import ai.x.play.json.Jsonx +import play.api.libs.json.OFormat case class ClientConfig(username: String, capiUrl: String, @@ -15,5 +16,5 @@ case class ClientConfig(username: String, ) object ClientConfig { - implicit val clientConfigFormat = Jsonx.formatCaseClass[ClientConfig] + implicit val clientConfigFormat: OFormat[ClientConfig] = Jsonx.formatCaseClass[ClientConfig] } diff --git a/app/model/ContributorInformation.scala b/app/model/ContributorInformation.scala index cc70b26d..bc9f46ca 100644 --- a/app/model/ContributorInformation.scala +++ b/app/model/ContributorInformation.scala @@ -36,7 +36,7 @@ case class ContributorInformation( object ContributorInformation { - implicit val contributorInformationFormat = Jsonx.formatCaseClass[ContributorInformation] + implicit val contributorInformationFormat: OFormat[ContributorInformation] = Jsonx.formatCaseClass[ContributorInformation] def apply(thriftContributorInformation: ThriftContributorInformation): ContributorInformation = ContributorInformation( diff --git a/app/model/EditionalisedPage.scala b/app/model/EditionalisedPage.scala index e98b892d..68e947bd 100644 --- a/app/model/EditionalisedPage.scala +++ b/app/model/EditionalisedPage.scala @@ -3,6 +3,7 @@ package model import ai.x.play.json.Jsonx import ai.x.play.json.Encoders.encoder import com.gu.tagmanagement.{EditionalisedPage => ThriftEditionalisedPage} +import play.api.libs.json.OFormat case class EditionalisedPage(path: String, pageId: Long) { def asThift = ThriftEditionalisedPage(path, pageId) @@ -10,7 +11,7 @@ case class EditionalisedPage(path: String, pageId: Long) { object EditionalisedPage { - implicit val editionalisedPageFormat = Jsonx.formatCaseClass[EditionalisedPage] + implicit val editionalisedPageFormat: OFormat[EditionalisedPage] = Jsonx.formatCaseClass[EditionalisedPage] def apply(thriftEditionalisedPage: ThriftEditionalisedPage): EditionalisedPage = EditionalisedPage(thriftEditionalisedPage.path, thriftEditionalisedPage.pageId) diff --git a/app/model/ExternalReferenceType.scala b/app/model/ExternalReferenceType.scala index 6a85a04b..784ccdf5 100644 --- a/app/model/ExternalReferenceType.scala +++ b/app/model/ExternalReferenceType.scala @@ -10,6 +10,7 @@ import play.api.libs.json.{Format, JsPath, JsValue, Json} import com.gu.tagmanagement.{Section => ThriftSection} import scala.util.control.NonFatal +import play.api.libs.json.OFormat case class ExternalReferenceType ( typeName: String, @@ -20,7 +21,7 @@ case class ExternalReferenceType ( object ExternalReferenceType extends Logging { - implicit val sectionFormat = Jsonx.formatCaseClass[ExternalReferenceType] + implicit val sectionFormat: OFormat[ExternalReferenceType] = Jsonx.formatCaseClass[ExternalReferenceType] def fromItem(item: Item) = try{ Json.parse(item.toJSON).as[ExternalReferenceType] diff --git a/app/model/HyperMedia.scala b/app/model/HyperMedia.scala index 241f79e0..4e05c179 100644 --- a/app/model/HyperMedia.scala +++ b/app/model/HyperMedia.scala @@ -4,7 +4,7 @@ import services.Config.conf case class LinkEntity(rel: String, href: String) -object LinkEntity { implicit val jsonWrites = Json.writes[LinkEntity] } +object LinkEntity { implicit val jsonWrites: OWrites[LinkEntity] = Json.writes[LinkEntity] } case class EmbeddedEntity[T](uri: String, data: Option[T] = None, @@ -59,7 +59,7 @@ object CollectionResponse { case class EmptyResponse(links: Option[List[LinkEntity]] = None) { def addLink(rel: String, href: String) = copy(links = Some(LinkEntity(rel, href) :: (links getOrElse Nil))) } -object EmptyResponse { implicit val jsonWrites = Json.writes[EmptyResponse] } +object EmptyResponse { implicit val jsonWrites: OWrites[EmptyResponse] = Json.writes[EmptyResponse] } object HyperMediaHelpers { def tagUri(id: Long): String = s"https://tagmanager.${conf.pandaDomain}/hyper/tags/${id}" diff --git a/app/model/ImageAsset.scala b/app/model/ImageAsset.scala index ea4c0af0..3981372e 100644 --- a/app/model/ImageAsset.scala +++ b/app/model/ImageAsset.scala @@ -3,6 +3,7 @@ package model import ai.x.play.json.Jsonx import ai.x.play.json.Encoders.encoder import com.gu.tagmanagement.{ImageAsset => ThriftImageAsset} +import play.api.libs.json.OFormat case class ImageAsset(imageUrl: String, width: Long, height: Long, mimeType: String) { def asThrift = ThriftImageAsset(imageUrl, width, height, mimeType) @@ -15,7 +16,7 @@ case class ImageAsset(imageUrl: String, width: Long, height: Long, mimeType: Str } object ImageAsset { - implicit val imageAssetFormat = Jsonx.formatCaseClass[ImageAsset] + implicit val imageAssetFormat: OFormat[ImageAsset] = Jsonx.formatCaseClass[ImageAsset] def apply(thriftImageAsset: ThriftImageAsset): ImageAsset = ImageAsset( imageUrl = thriftImageAsset.imageUrl, diff --git a/app/model/PaidContentInformation.scala b/app/model/PaidContentInformation.scala index e93d3dc7..e743c24b 100644 --- a/app/model/PaidContentInformation.scala +++ b/app/model/PaidContentInformation.scala @@ -3,6 +3,7 @@ package model import ai.x.play.json.Jsonx import ai.x.play.json.Encoders.encoder import com.gu.tagmanagement.{PaidContentInformation => ThriftPaidContentInformation} +import play.api.libs.json.OFormat case class PaidContentInformation(paidContentType: String, campaignColour: Option[String] = None) { @@ -19,7 +20,7 @@ case class PaidContentInformation(paidContentType: String, campaignColour: Optio object PaidContentInformation { - implicit val paidContentInformationFormat = Jsonx.formatCaseClass[PaidContentInformation] + implicit val paidContentInformationFormat: OFormat[PaidContentInformation] = Jsonx.formatCaseClass[PaidContentInformation] def apply(thriftPaidContentInformation: ThriftPaidContentInformation): PaidContentInformation = PaidContentInformation( diff --git a/app/model/PublicationInformation.scala b/app/model/PublicationInformation.scala index 8d8abfdf..c0e735ad 100644 --- a/app/model/PublicationInformation.scala +++ b/app/model/PublicationInformation.scala @@ -24,7 +24,7 @@ case class PublicationInformation( object PublicationInformation { - implicit val publicationInformationFormat = Jsonx.formatCaseClassUseDefaults[PublicationInformation] + implicit val publicationInformationFormat: OFormat[PublicationInformation] = Jsonx.formatCaseClassUseDefaults[PublicationInformation] def apply(thriftPublicationInformation: ThriftPublicationInformation): PublicationInformation = PublicationInformation( diff --git a/app/model/Reference.scala b/app/model/Reference.scala index bf2d76d3..affc64d4 100644 --- a/app/model/Reference.scala +++ b/app/model/Reference.scala @@ -25,7 +25,7 @@ case class Reference(`type`: String, value: String, capiType: Option[String]) { object Reference { - implicit val referenceFormat = Jsonx.formatCaseClass[Reference] + implicit val referenceFormat: OFormat[Reference] = Jsonx.formatCaseClass[Reference] def apply(thriftReference: ThriftReference): Reference = Reference(thriftReference.`type`, thriftReference.value, thriftReference.capiType) diff --git a/app/model/Sponsorship.scala b/app/model/Sponsorship.scala index 82347855..53e9d59e 100644 --- a/app/model/Sponsorship.scala +++ b/app/model/Sponsorship.scala @@ -99,7 +99,7 @@ case class DenormalisedSponsorship ( object DenormalisedSponsorship { - implicit val denormalisedSponsorshipFormat = Jsonx.formatCaseClass[DenormalisedSponsorship] + implicit val denormalisedSponsorshipFormat: OFormat[DenormalisedSponsorship] = Jsonx.formatCaseClass[DenormalisedSponsorship] def apply(s: Sponsorship): DenormalisedSponsorship = { new DenormalisedSponsorship( diff --git a/app/model/Tag.scala b/app/model/Tag.scala index a95cd810..b4028fda 100644 --- a/app/model/Tag.scala +++ b/app/model/Tag.scala @@ -248,7 +248,7 @@ case class DenormalisedTag ( object DenormalisedTag{ - implicit val tagFormat = Jsonx.formatCaseClassUseDefaults[DenormalisedTag] + implicit val tagFormat: OFormat[DenormalisedTag] = Jsonx.formatCaseClassUseDefaults[DenormalisedTag] def apply(t: Tag): DenormalisedTag = DenormalisedTag( id = t.id, diff --git a/app/model/TagSearchResult.scala b/app/model/TagSearchResult.scala index 5f988797..4b9c6eb0 100644 --- a/app/model/TagSearchResult.scala +++ b/app/model/TagSearchResult.scala @@ -2,9 +2,10 @@ package model import ai.x.play.json.Jsonx import ai.x.play.json.Encoders.encoder +import play.api.libs.json.OFormat case class TagSearchResult(tags: List[Tag], count: Int) object TagSearchResult { - implicit val tagSearchResultFormat = Jsonx.formatCaseClassUseDefaults[TagSearchResult] + implicit val tagSearchResultFormat: OFormat[TagSearchResult] = Jsonx.formatCaseClassUseDefaults[TagSearchResult] } diff --git a/app/model/TrackingInformation.scala b/app/model/TrackingInformation.scala index 1196f853..b13f239d 100644 --- a/app/model/TrackingInformation.scala +++ b/app/model/TrackingInformation.scala @@ -18,7 +18,7 @@ case class TrackingInformation(trackingType: String) { object TrackingInformation { - implicit val trackingFormat = Jsonx.formatCaseClass[TrackingInformation] + implicit val trackingFormat: OFormat[TrackingInformation] = Jsonx.formatCaseClass[TrackingInformation] def apply(thriftTrackingInformation: ThriftTrackingInformation): TrackingInformation = TrackingInformation( diff --git a/app/model/command/BatchTagCommand.scala b/app/model/command/BatchTagCommand.scala index f2509e9e..79f57674 100644 --- a/app/model/command/BatchTagCommand.scala +++ b/app/model/command/BatchTagCommand.scala @@ -7,6 +7,7 @@ import ai.x.play.json.Encoders.encoder import repositories._ import scala.concurrent.{Future, ExecutionContext} +import play.api.libs.json.OFormat case class BatchTagCommand(contentIds: List[String], toAddToTop: Option[Long], toAddToBottom: List[Long], toRemove: List[Long]) extends Command { type T = Unit @@ -37,5 +38,5 @@ case class BatchTagCommand(contentIds: List[String], toAddToTop: Option[Long], t } object BatchTagCommand { - implicit val batchTagCommandFormat = Jsonx.formatCaseClassUseDefaults[BatchTagCommand] + implicit val batchTagCommandFormat: OFormat[BatchTagCommand] = Jsonx.formatCaseClassUseDefaults[BatchTagCommand] } diff --git a/app/model/command/CreateTagCommand.scala b/app/model/command/CreateTagCommand.scala index f0b4dcd7..9e99325c 100644 --- a/app/model/command/CreateTagCommand.scala +++ b/app/model/command/CreateTagCommand.scala @@ -17,6 +17,7 @@ import play.api.Logging import services.KinesisStreams import scala.concurrent.{Future, ExecutionContext} +import play.api.libs.json.OFormat case class InlinePaidContentSponsorshipCommand( validFrom: Option[DateTime], @@ -53,7 +54,7 @@ case class InlinePaidContentSponsorshipCommand( } object InlinePaidContentSponsorshipCommand { - implicit val inlinePaidContentSponsorshipFormat = Jsonx.formatCaseClassUseDefaults[InlinePaidContentSponsorshipCommand] + implicit val inlinePaidContentSponsorshipFormat: OFormat[InlinePaidContentSponsorshipCommand] = Jsonx.formatCaseClassUseDefaults[InlinePaidContentSponsorshipCommand] } case class CreateTagCommand( @@ -207,5 +208,5 @@ case class CreateTagCommand( object CreateTagCommand { - implicit val createTagCommandFormat = Jsonx.formatCaseClassUseDefaults[CreateTagCommand] + implicit val createTagCommandFormat: OFormat[CreateTagCommand] = Jsonx.formatCaseClassUseDefaults[CreateTagCommand] } diff --git a/app/model/forms/SpreadSheet.scala b/app/model/forms/SpreadSheet.scala index 8df49842..e3d0bd09 100644 --- a/app/model/forms/SpreadSheet.scala +++ b/app/model/forms/SpreadSheet.scala @@ -34,6 +34,6 @@ object GetSpreadSheet { } } - implicit val filterFormat = Json.reads[SpreadSheetFilter] - implicit val format = Json.reads[GetSpreadSheet] + implicit val filterFormat: Reads[SpreadSheetFilter] = Json.reads[SpreadSheetFilter] + implicit val format: Reads[GetSpreadSheet] = Json.reads[GetSpreadSheet] } diff --git a/app/model/image.scala b/app/model/image.scala index 9db46090..18b3f582 100644 --- a/app/model/image.scala +++ b/app/model/image.scala @@ -19,7 +19,7 @@ case class Image(imageId: String, assets: List[ImageAsset]) { } object Image { - implicit val imageFormat = Jsonx.formatCaseClass[Image] + implicit val imageFormat: OFormat[Image] = Jsonx.formatCaseClass[Image] def apply(thriftImage: ThriftImage): Image = Image( imageId = thriftImage.imageId, diff --git a/app/model/jobs/Step.scala b/app/model/jobs/Step.scala index 13859061..2633bc38 100644 --- a/app/model/jobs/Step.scala +++ b/app/model/jobs/Step.scala @@ -172,7 +172,7 @@ object Step extends Logging { } } - implicit val stepFormat = Format(stepReads, stepWrites) + implicit val stepFormat: Format[Step] = Format(stepReads, stepWrites) } // Step status is required so we know which steps require rollback diff --git a/app/modules/sponsorshiplifecycle/SponsorshipLifecycleModule.scala b/app/modules/sponsorshiplifecycle/SponsorshipLifecycleModule.scala index 807f29ac..caed5f45 100644 --- a/app/modules/sponsorshiplifecycle/SponsorshipLifecycleModule.scala +++ b/app/modules/sponsorshiplifecycle/SponsorshipLifecycleModule.scala @@ -86,7 +86,7 @@ class SponsorshipLauncher(implicit ec: ExecutionContext) extends AbstractSchedul } class SponsorshipExpirer(implicit ec: ExecutionContext) extends AbstractScheduledService with Logging { - implicit val username = Some("Sponsorship expirer") + implicit val username: Some[String] = Some("Sponsorship expirer") override def runOneIteration(): Unit = try {