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

Apply inspections #588

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Mar 29, 2024

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

  • Every case class has been made final unless it causes the Scala 2 specific The 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 a case class they can break properties of case 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 our case classes I would argue this is a good thing as we should be aware of why someone is doing this.
  • For custom extensions i.e. extending ExtensionId explicit types have been added to the overridden lookup 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 the lookup method then it just defaults to ExtensionId[_ <: Extension] which less specific/useful than lets say GoogleExt.type (which extends ExtensionId[GoogleExt] and ExtensionIdProvider). MiMa filters have been added to help this.
  • private/internal class has been made final and/or private where it works to do so

@He-Pin
Copy link
Member

He-Pin commented Apr 7, 2024

I would suggest commit one by one.

@He-Pin
Copy link
Member

He-Pin commented Apr 7, 2024

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

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Apr 7, 2024

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
Copy link
Member

He-Pin commented Apr 7, 2024

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
Copy link
Contributor Author

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
Copy link
Member

He-Pin commented Apr 7, 2024

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

@samueleresca
Copy link
Member

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

@mdedetrich
Copy link
Contributor Author

@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
Copy link
Contributor Author

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"

Copy link
Member

@samueleresca samueleresca left a 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.

@mdedetrich mdedetrich force-pushed the apply-inspections branch 2 times, most recently from fd30964 to 484a69b Compare April 14, 2024 08:19
Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Lgtm, great

@mdedetrich
Copy link
Contributor Author

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!

@raboof raboof marked this pull request as draft April 25, 2024 13:55
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.

None yet

3 participants