-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix scala 2.13 warnings #260
Conversation
Unfortunately `lazyZip` is not supported by Scala 2.12 so for now we need a shim
Auto-application to `()` is deprecated. Supply the empty argument list `()` explicitly to invoke method getFronts
copyArrayToImmutableIndexedSeq in class LowPriorityImplicits2 is deprecated (since 2.13.0): Implicit conversions from Array to immutable.IndexedSeq are implemented by copying; Use the more efficient non-copying ArraySeq.unsafeWrapArray or an explicit toIndexedSeq call
aea6704
to
1d1b8f5
Compare
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.
Don't forget to fix the tests! I'm seeing errors like this when running Test / compile
error] /Users/roberto/guardian/facia-scala-client/fapi-client/src/test/scala/com/gu/facia/api/utils/ResolvedMetaDataTest.scala:167:9: symbol literal is deprecated; use Symbol("showMainVideo") instead
[error] 'showMainVideo (false))
[error] ^
I think this repo might need some CI 😃
@@ -58,7 +59,7 @@ object FAPI { | |||
) | |||
collectionConfigs = collectionConfigJsons.map(CollectionConfig.fromCollectionJson) | |||
} yield { | |||
(collectionIds, collectionsJsons, collectionConfigs).zipped.toList.map { case (collectionId, collectionJson, collectionConfig) => | |||
collectionIds.lazyZip(collectionsJsons).lazyZip(collectionConfigs).toList.map { case (collectionId, collectionJson, collectionConfig) => |
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.
Just want to check, would it be possible to use scala-collection-compat
rather than building your own shim?
It looks like ws lazyZip xs lazyZip ys
is supported by scala-collection-compat
?
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.
Amazing, I looked for this but only found https://github.com/giabao/scala-collection-compat-lazyzip/ which didn't look maintained.
Thanks, fixed. |
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.
Awesome work man!
fapi-client/src/main/scala/com/gu/facia/api/contentapi/ContentApi.scala
Outdated
Show resolved
Hide resolved
@@ -92,7 +92,7 @@ class IntegrationTest extends FreeSpec with Matchers with ScalaFutures with Opti | |||
|
|||
"will use the provided function to adjust the query used to hydrate content" ignore { | |||
val adjust: AdjustSearchQuery = q => q.showTags("tone") | |||
val collection = collectionResponse.right.get | |||
val collection = collectionResponse.toOption.get |
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.
Not essential, just wanted to note that if you update to current ScalaTest (which would involve classname/imports updates), you can use the EitherValues
trait and do collectionResponse.value
rather than .toOption.get
(thanks to scalatest/scalatest#1895).
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.
Nice, this would be a good follow up improvement, but I'll leave it out since this is already a bigger PR than I'd hoped.
What does this change?
Fixes the build, which was broken by #259. Treating compiler warnings as errors is a nice idea, but I only tested it on Scala 2.12. This project is set up to cross compile to Scala 2.13 too. Scala 2.13 deprecates a number of features, which this PR resolves.