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

Build with Scala 2.13 #1530

Merged
merged 15 commits into from
Feb 27, 2019
Merged

Build with Scala 2.13 #1530

merged 15 commits into from
Feb 27, 2019

Conversation

2m
Copy link
Member

@2m 2m commented Feb 26, 2019

Fixes

Towards #828.

Purpose

This enables cross building for Scala 2.13.0-M5. Connectors that have either missing dependencies or are problematic to cross-build, have cross-publishing disabled.

throw new UnsupportedOperationException("Convert to map from iterator to support removing elements.")

override def updated[V1 >: V](key: K, value: V1): scala.collection.immutable.Map[K, V1] =
throw new UnsupportedOperationException("Convert to map from iterator to support updating elements.")
Copy link

@helena helena Feb 26, 2019

Choose a reason for hiding this comment

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

I always wonder about code like this - declaring one return type but only throwing. I understand why this versus a immutable.Map.empty[K,V] but prefer the return is as declared when possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea. I agree. I'll implement the method or will try to get rid of the MapOverTraversable all together.

"software.amazon.awssdk" % "sns" % AwsSdk2Version excludeAll (ExclusionRule(
organization = "software.amazon.awssdk",
name = "netty-nio-client"
), ExclusionRule(organization = "io.netty")), // ApacheV2
Copy link

Choose a reason for hiding this comment

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

Perhaps keep the wrapping the same, license on the similar relation to what's being licensed in these two, for convention reasons, since they are now different.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, scalafmt reformatted it this way...

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Some nitpicks but generally looks good. Do we want to create separate issues for each module not supporting 2.13 to track what's holding that back?

tableName,
columnFamilies.asScala.to[immutable.Seq],
converter.asScala(_).asScala.to[immutable.Seq])
HTableSettings(conf, tableName, columnFamilies.asScala.toIndexedSeq, converter.asScala(_).asScala.toIndexedSeq)
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to have the newlines here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its scalafmt. :)

val ScalaVersions = Seq("2.12.7", "2.11.12")
val Scala212 = "2.12.7"
val Scala213 = "2.13.0-M5"
val ScalaVersions = Seq(Scala212, "2.11.12", Scala213)
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to also add a constant for 211 for consistency ;)

@@ -48,6 +48,11 @@ private class MapOverTraversable[A, K, V](source: Traversable[A], fKey: A => K,

override def iterator: Iterator[(K, V)] = source.toIterator.map(a => (fKey(a), fValue(a)))

def remove(key: K): scala.collection.immutable.Map[K, V] =
throw new UnsupportedOperationException("Convert to map from iterator to support removing elements.")
Copy link
Member

Choose a reason for hiding this comment

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

Traversable is not really an iterator, since it should be traversable multiple times (otherwise it'd only be a TraversableOnce) - couldn't we do something like new MapOverTraversable(source.filter(k => fKey(k) != key), fKey, fValue) here?

throw new UnsupportedOperationException("Convert to map from iterator to support removing elements.")

override def updated[V1 >: V](key: K, value: V1): scala.collection.immutable.Map[K, V1] =
throw new UnsupportedOperationException("Convert to map from iterator to support updating elements.")
Copy link
Member

Choose a reason for hiding this comment

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

Here we'd have to actually convert all elements of the traversable... perhaps indeed best to not support that since it might be surprising performance-wise?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to remove MapOverTraversable and go with collection conversions. Favoring no exceptions rather than slightly faster implementation.

Copy link

@helena helena left a comment

Choose a reason for hiding this comment

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

LGTM

@2m
Copy link
Member Author

2m commented Feb 27, 2019

Addressed the feedback. Tickets for connectors that have cross-publishing disabled incoming.

@2m
Copy link
Member Author

2m commented Feb 27, 2019

Fixes #828

@2m 2m added this to the 1.0-M3 milestone Feb 27, 2019
@2m 2m merged commit a0a5c69 into akka:master Feb 27, 2019
@2m 2m deleted the wip-scala-213-2m branch February 27, 2019 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants