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.

Sign up for GitHub

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

Jump to bottom

ArC: remove deprecated @AlternativePriority #31033

Merged
merged 1 commit into from
Feb 10, 2023
Merged

ArC: remove deprecated @AlternativePriority #31033

merged 1 commit into from
Feb 10, 2023

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Feb 9, 2023

Resolves #30963

@Ladicek Ladicek requested review from mkouba and manovotn February 9, 2023 10:49
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 9, 2023

/cc @brunobat (micrometer), @ebullient (micrometer)

@Ladicek Ladicek mentioned this pull request Feb 9, 2023
Merged
mkouba
mkouba approved these changes Feb 9, 2023
View reviewed changes
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@Ladicek Ladicek mentioned this pull request Feb 9, 2023
Closed
@quarkus-bot

This comment has been minimized.

Sign in to view
@Ladicek Ladicek force-pushed the remove-alternativepriority branch from ad9fe44 to 86c7988 Compare February 9, 2023 11:06
@github-actions
Copy link

github-actions bot commented Feb 9, 2023 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@quarkus-bot
Copy link

quarkus-bot bot commented Feb 9, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gastaldi
Copy link
Contributor

gastaldi commented Feb 9, 2023 •
edited
Loading

Would it make sense to also update the migration script to perform the substitution? Like it was done in

@manovotn
Copy link
Contributor

manovotn commented Feb 9, 2023

Would it make sense to also update the migration script to perform the substitution? Like it was done in

Can the rewrite actually change one annotation into two?
And can it preserve annotation value in one of them?
Genuine questions, never really tried anything else than namespace change or swapping one annotation for another without values :)

Because what we would need is @AlternativePriority(1) -> @Alternative @Priority(1)

@gastaldi
Copy link
Contributor

gastaldi commented Feb 9, 2023 •
edited
Loading

@manovotn that's a good question, I'll investigate and write one if not available and update the migration script in a later PR then 👍🏻

UPDATE: there is a discussion about this in https://rewriteoss.slack.com/archives/C01AB6L98TC/p1675986262428799

@gastaldi gastaldi merged commit 3bb418d into quarkusio:main Feb 10, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 10, 2023
@Ladicek Ladicek deleted the remove-alternativepriority branch February 10, 2023 08:17
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 10, 2023

It didn't occur to me that we could add a migration for this, but it's certainly worth trying! Thank you! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebullient ebullient ebullient approved these changes

@mkouba mkouba mkouba approved these changes

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/documentation area/health area/metrics area/oidc area/reactive-messaging area/smallrye release/breaking-change
Projects
None yet
Milestone
3.0.0.Alpha5
Development

Successfully merging this pull request may close these issues.

ArC: remove deprecated @AlternativePriority
6 participants
@Ladicek @gastaldi @manovotn @ebullient @mkouba @gsmet