Skip to content
This repository has been archived by the owner on Mar 11, 2023. It is now read-only.

Add Scala 2.13 support #275

Merged

Conversation

rtyley
Copy link
Contributor

@rtyley rtyley commented Oct 21, 2019

This PR updates twitter4s to compile with Scala 2.13 by default, while still cross-compiling to Scala 2.12 & 2.11.

Although Scala 2.13.1 has been released, this change sticks with 2.13.0, as scoverage has not yet released an update compatible with that latest version (see scoverage/sbt-scoverage#295, scoverage/sbt-scoverage#299) - the current latest release of scoverage, 1.6.0, only works with Scala 2.13.0.

Migration issues addressed with this update:

  • Underscore (_) is no longer a valid identifer for vals
  • Map.mapValues() now returns a MapView rather than a Map
  • .to can no longer infer what resulting collection type you want

…atype OSS releases)

Use https instead of http for JGit resolver, otherwise sbt gives security warnings.

See also:

https://www.scala-sbt.org/1.x/docs/Resolvers.html#Predefined+resolvers
@@ -67,7 +67,7 @@ private[twitter4s] class OAuth1Provider(consumerToken: ConsumerToken, accessToke
val method = request.method.name.urlEncoded
val baseUrl = request.uri.endpoint.urlEncoded
val oauthParams = oauth2Params.map {
case (k, v) =>
case (k: String, v: String) =>
if (k == "oauth_callback") k -> v.urlEncoded
else k -> v
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scala 2.13 Maps actually have a new updatedWith() method for performing this operation!
So when support for 2.11 & 2.12 is dropped, this code can become a bit smaller: ✨

val oauthParams = oauth2Params.updatedWith("oauth_callback")(_.map(_.urlEncoded))

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, I'll keep it in mind when 2.11 & 2.12 will no longer be in the picture :)

@@ -18,7 +18,7 @@ trait Client extends OAuthClient {
protected def sendAndReceive[T](request: HttpRequest, f: HttpResponse => Future[T])(
implicit system: ActorSystem,
materializer: Materializer): Future[T] = {
implicit val _ = request
implicit val r: HttpRequest = request
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Underscore (_) is no longer a valid identifier for vals, so we have to give it a valid name (even if it's just r) - see also scala/bug#10384, scala/bug#7691 & scala/scala#6218.

Copy link
Owner

Choose a reason for hiding this comment

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

oh nice! I didb't know :)

@@ -30,7 +30,7 @@ private[twitter4s] class OAuth1Provider(consumerToken: ConsumerToken, accessToke
val params = basicOAuth2Params(callback)
for {
signature <- oauth1Signature(params)
} yield (params + signature).mapValues(_.urlEncoded)
} yield (params + signature).mapValues(_.urlEncoded).toMap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Map.mapValues() now returns a MapView rather than a Map - we usually want a Map though, so we can just add in a .toMap to the end of the expression - it's harmless in Scala 2.12, and allows the code to compile in Scala 2.13.

Map.mapValues() itself is deprecated in Scala 2.13, but using the new recommended alternative doesn't work natively in Scala 2.12.

https://docs.scala-lang.org/overviews/core/collections-migration-213.html#what-are-the-breaking-changes

See also scala/bug#10919& scala/scala#7014.

@@ -42,6 +42,6 @@ trait BodyEncoder {

// TODO - improve performance with Macros?
private def asMap(cc: Product): Map[String, Any] =
cc.getClass.getDeclaredFields.map(_.getName).zip(cc.productIterator.to).toMap
cc.getClass.getDeclaredFields.map(_.getName).zip(cc.productIterator.toSeq).toMap
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.to can no longer infer what resulting collection type to produce - this is part of CanBuildFrom going away in Scala 2.13:

https://www.scala-lang.org/blog/2018/06/13/scala-213-collections.html#life-without-canbuildfrom

This project now compiles with Scala 2.13 by default, while still
cross-compiling to Scala 2.12 & 2.11.

Although Scala 2.13.1 has been released, this change sticks with 2.13.0,
as `scoverage` has not yet released an update compatible with 2.13.1:

scoverage/sbt-scoverage#295
scoverage/scalac-scoverage-plugin#283
scoverage/sbt-scoverage#299


Migration issues addressed with the update to Scala 2.13:

### Underscore (`_`) is no longer a valid identifer for `val`s

Using underscore like this is no longer allowed...

```
implicit val _ = ...
```

...we have to give the `val` a valid name (even if it's just `v`):


```
implicit val v = ...
```

See also:

* scala/bug#10384
* scala/bug#7691
* scala/scala#6218

### `Map.mapValues()` now returns a `MapView` rather than a `Map`

We usually want a `Map`, so we can just add in a `.toMap` to the end of
the expression - it's harmless in Scala 2.12, and allows the code to
compile in Scala 2.13. `Map.mapValues()` itself is deprecated in Scala
2.13, but using the new recommended alternative doesn't work in Scala 2.12.

https://docs.scala-lang.org/overviews/core/collections-migration-213.html#what-are-the-breaking-changes

See also:

* scala/bug#10919
* scala/scala#7014

### `.to` can no longer infer what resulting collection type you want

This is all part of `CanBuildFrom` going away in Scala 2.13:

https://www.scala-lang.org/blog/2018/06/13/scala-213-collections.html#life-without-canbuildfrom
@codecov
Copy link

codecov bot commented Oct 21, 2019

Codecov Report

Merging #275 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
- Coverage   92.68%   92.55%   -0.14%     
==========================================
  Files          77       76       -1     
  Lines        1176     1142      -34     
  Branches       10        9       -1     
==========================================
- Hits         1090     1057      -33     
+ Misses         86       85       -1
Impacted Files Coverage Δ
...r4s/http/serializers/StreamingMessageFormats.scala 95.12% <ø> (-0.12%) ⬇️
...anielasfregola/twitter4s/http/clients/Client.scala 40% <ø> (ø) ⬆️
...sfregola/twitter4s/http/oauth/OAuth1Provider.scala 100% <100%> (ø) ⬆️
...egola/twitter4s/http/marshalling/BodyEncoder.scala 100% <100%> (ø) ⬆️
...anielasfregola/twitter4s/util/Configurations.scala 60% <0%> (-6.67%) ⬇️
...regola/twitter4s/exceptions/TwitterException.scala 71.42% <0%> (-3.58%) ⬇️
...ients/streaming/statuses/TwitterStatusClient.scala 92.3% <0%> (-0.55%) ⬇️
...s/http/clients/rest/media/TwitterMediaClient.scala 96.87% <0%> (-0.1%) ⬇️
...st/directmessages/TwitterDirectMessageClient.scala 100% <0%> (ø) ⬆️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f26e50b...34571e8. Read the comment docs.

Copy link
Owner

@DanielaSfregola DanielaSfregola left a comment

Choose a reason for hiding this comment

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

Hi @rtyley,
thank you very much for your super useful contribution!!!! :D

LGTM

I'll try to release a new version as soon as possible

@DanielaSfregola DanielaSfregola merged commit e13b6f6 into DanielaSfregola:master Oct 31, 2019
@DanielaSfregola DanielaSfregola added this to the 6.2 milestone Oct 31, 2019
@rtyley
Copy link
Contributor Author

rtyley commented Oct 31, 2019

Thanks so much, we really appreciate it!

@DanielaSfregola
Copy link
Owner

twitter4s 6.2 is on its way to maven central!

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

Successfully merging this pull request may close these issues.

2 participants