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

Feat/support all annotations in scala 3 #1471

Open
wants to merge 3 commits into
base: series/4.x
Choose a base branch
from

Conversation

ThijsBroersen
Copy link

@ThijsBroersen ThijsBroersen commented Oct 16, 2024

This PR add support for annotations in Scala 3 on all platforms. The added tests are used for testing all platforms and scala versions to ensure unambiguous behaviour.
This PR introduces the suffix annotation as a replacement for postfix (to deprecate). Semantics of postfix to not properly reflect the feature.

@ThijsBroersen ThijsBroersen requested a review from a team as a code owner October 16, 2024 14:36
@ThijsBroersen ThijsBroersen marked this pull request as draft October 16, 2024 14:36
@ThijsBroersen ThijsBroersen force-pushed the feat/support-all-annotations-in-scala-3 branch from cc205a2 to dc642a7 Compare November 15, 2024 23:26
@ThijsBroersen ThijsBroersen force-pushed the feat/support-all-annotations-in-scala-3 branch from dc642a7 to d2cdce5 Compare November 15, 2024 23:50
@ThijsBroersen ThijsBroersen changed the base branch from chore/build/refactor-to-projectmatrix-and-bump-all to series/4.x November 15, 2024 23:51
@ThijsBroersen ThijsBroersen marked this pull request as ready for review November 15, 2024 23:52
@ThijsBroersen ThijsBroersen reopened this Nov 15, 2024
@ThijsBroersen
Copy link
Author

ThijsBroersen commented Nov 16, 2024

@kyri-petrou @afsalthaj
I see mima checks got recently added. I wanted to share KeyModifier between Scala 2 and 3 but somehow that broke mima. I now duplicated the code.

How important is mima atm? I changed Macros to AnnotationMacros and made it private to the magnolia package. I don't think it should be public api.
Should the CI Mima step hard fail atm, or warn? Or perhaps a manual approval? (e.g. https://github.com/marketplace/actions/manual-workflow-approval)?

Comment on lines +340 to +342
originalKey.fold(desc)(k => desc.nested(modifyKey(k)))
// case keys => keys.view.map(k => desc.nested(modifyKey(k))).reduce(_ orElse _) // Looks like the Scala 3 implementation modifies alternative names while the Scala 2 implementations treats them as is.
case keys => keys.view.map(k => desc.nested(k)).reduce(_ orElse _)
Copy link
Author

Choose a reason for hiding this comment

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

please review this carefully, because I think L341 should be enabled and L342 disabled. This changes the existing behaviour only to match the Scala 2 behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

L340 I already adjusted.

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.

1 participant