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

Rename Property Plugin from PropertyRequiresPlugin to ConfigPropertyPlugin #572

Merged
merged 45 commits into from
Jun 11, 2024

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented May 22, 2024

We use it for more than the @RequiresX annotations. Also I don't like the name.

@SentryMan SentryMan requested a review from rbygrave May 22, 2024 12:49
@SentryMan SentryMan self-assigned this May 22, 2024
@SentryMan SentryMan added this to the 10.0 milestone May 22, 2024
@rob-bygrave rob-bygrave changed the title Rename Property Plugin Rename Property Plugin from PropertyRequiresPlugin to ConfigPropertyPlugin Jun 7, 2024
@rbygrave
Copy link
Contributor

rbygrave commented Jun 8, 2024

I'm pondering if we just go for a simple breaking change here. As in, likely very very few people have implemented this interface (it hasn't been around for a long time etc). So I think it might be acceptable and simple and less messy to just go with a simple rename and breaking change. Pondering ...

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jun 8, 2024

I'm pondering if we just go for a simple breaking change here.

It'll break every single one of the other avaje plugins that uses configuration if we do a clean break. Like the Jsonb Inject plugin for example

@rbygrave rbygrave merged commit 1bf404c into avaje:master Jun 11, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants