-
Notifications
You must be signed in to change notification settings - Fork 33
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
Apply inspections #588
base: main
Are you sure you want to change the base?
Apply inspections #588
Conversation
e53d84a
to
d84dc63
Compare
d84dc63
to
405df96
Compare
405df96
to
505ca92
Compare
I would suggest commit one by one. |
Another thing is case class can be extended in Java, this is not binary compatible |
It doesn't matter if it's Java or Scala, case classes shouldn't ever be extended because the usage of case class implies certain properties that can be broken if a user overrides them. If a case class is intended to be overridden, you should using a normal class instead.
The entire Pekko 1.1.x series is not binary compatible with Pekko 1.0.x, that's why this is being done now |
I think we should add annotations. @INTERNAL_API and/Or @doNotInherent in 1.1 cross the projects, and mark it final in 1.2 or 2.0 what ever. In pekko, there are some classes which is not annotated with these annotations but just comments internal api.sorry, typing from phone |
Alpakka has never had any prpper binary compatibility guarantees, it's simply not practical with a monorepo with 20+ integrations and we inherited that. Due to the fact that we have done major underlying library updates causing breaking changes to many connectors already, the entire release will be classed as breaking. The plan is that once that release is made, we will start splitting off the connectors into their own repos so we don't get the issues stemming from a monorepo |
Okay, but we should add those annotations in pekko core project |
csv/src/main/scala/org/apache/pekko/stream/connectors/csv/impl/CsvToMapJavaStage.scala
Outdated
Show resolved
Hide resolved
elasticsearch/src/test/java/docs/javadsl/ElasticsearchV5Test.java
Outdated
Show resolved
Hide resolved
google-cloud-pub-sub/src/test/java/docs/javadsl/ExampleUsageJava.java
Outdated
Show resolved
Hide resolved
hdfs/src/main/scala/org/apache/pekko/stream/connectors/hdfs/model.scala
Outdated
Show resolved
Hide resolved
testkit/src/main/scala/org/apache/pekko/stream/connectors/testkit/CapturingAppender.scala
Outdated
Show resolved
Hide resolved
I had a quick skim through and left few minor comments. Is this change a good candidate for the |
505ca92
to
c36e186
Compare
@samueleresca Thanks, I have just force pushed with all of the changes fixed. I guess that technically speaking some of the changes can be added to I can do this once all of the other issues with the PR have been resolved. |
We can do this once we start splitting off the connectors into their own repo's as it will make sense then. The idea is that this release of pekko-connectors will the last of the "massive breaking changes" |
c36e186
to
746aa9d
Compare
746aa9d
to
94a2a83
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.
looks good to me now.
fd30964
to
484a69b
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.
Lgtm, great
FYI don't merge this PR, I am in the process of splitting out the pure formatting changes into a separate commit so I can add it to |
484a69b
to
9479cd1
Compare
This is a PR that applies a large set of inspections/syntax/formatting changes. In addition since next release of pekko-connectors will be a breaking change, this opens up additional improvements we can make which are noted below
case class
has been madefinal
unless it causes the Scala 2 specificThe outer reference in this type test cannot be checked at run time.
error (this has been solved in Scala 3 but its not worth splitting the code base over this). This is done because idiomatically in Scala,case class
's should always be final because if you allow users to extend acase class
they can break properties ofcase class
that you rely on (i.e. you can break structural equality by overriding equals). This shouldn't cause any real world problems, and if it does (i.e. someone is extending by subclassing one of ourcase classes
I would argue this is a good thing as we should be aware of why someone is doing this.ExtensionId
explicit types have been added to the overriddenlookup
methods which are more specific/correct than leaving out the explicit type ascription. This is needed because if you don't apply a type to thelookup
method then it just defaults toExtensionId[_ <: Extension]
which less specific/useful than lets sayGoogleExt.type
(which extendsExtensionId[GoogleExt]
andExtensionIdProvider
). MiMa filters have been added to help this.class
has been made final and/or private where it works to do so