apache / pekko-connectors

Apache Pekko Connectors is a Reactive Enterprise Integration library for Java and Scala, based on Reactive Streams and Apache Pekko.
https://pekko.apache.org/
Apache License 2.0
62 stars 31 forks source link

Apply inspections #588

Open mdedetrich opened 6 months ago

mdedetrich commented 6 months ago

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

He-Pin commented 6 months ago

I would suggest commit one by one.

He-Pin commented 6 months ago

Another thing is case class can be extended in Java, this is not binary compatible

mdedetrich commented 6 months ago

Another thing is case class can be extended in Java

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.

this is not binary compatible

The entire Pekko 1.1.x series is not binary compatible with Pekko 1.0.x, that's why this is being done now

He-Pin commented 6 months ago

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

mdedetrich commented 6 months ago

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

He-Pin commented 6 months ago

Okay, but we should add those annotations in pekko core project

samueleresca commented 6 months ago

I had a quick skim through and left few minor comments. Is this change a good candidate for the .git-blame-ignore-revs?

mdedetrich commented 6 months ago

@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 .git-blame-ignore-revs but I would have to manually seperate the pure syntax changes with the non syntax ones and that would take a bit.

I can do this once all of the other issues with the PR have been resolved.

mdedetrich commented 6 months ago

Okay, but we should add those annotations in pekko core project

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"

mdedetrich commented 5 months ago

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 .git-blame-ignore-revs and also cherry pick it into 1.0.x, stay tuned!