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

Add support for adding build and settings Gradle plugins #2597

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Add support for adding build and settings Gradle plugins #2597

merged 1 commit into from
Jan 4, 2023

Conversation

shanman190
Copy link
Contributor

No description provided.

@sambsnyd sambsnyd merged commit fdb04df into openrewrite:main Jan 4, 2023
@knutwannheden
Copy link
Contributor

@shanman190 Just for my understanding: Why do the new recipes and visitors declare an option named pluginIdPattern rather than just pluginId? AFAICT it ends up getting used by FindPlugins. Even though there the parameter of FindPlugins#find() is indeed also called pluginIdPattern, the corresponding option (@Option annotated field) of FindPlugins is called pluginId and there isn't any pattern matching taking place.

While I can see how a pattern could make sense for FindPlugins, I would in the case of AddBuildPlugin and AddSettingPlugin have expected the option to be called pluginId, since its purpose is to specify the ID of the plugin to add.


@Override
public String getDescription() {
return "Add a Gradle settings plugin to `build.gradle(.kts)`.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably read settings.gradle(.kts).


@Value
@EqualsAndHashCode(callSuper = true)
public class AddSettingPlugin extends Recipe {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if AddSettingsPlugin wouldn't be the more appropriate name.

@shanman190
Copy link
Contributor Author

@knutwannheden, honestly, was copy and paste for the options initially from another recipe. I updated the description, but forgot to update the parameter name itself. 😅 I've made the updates that you've mentioned and fixed the test issue with PR #2599.

The visitor was actually existing, but I've updated it's parameter as it more aligns to the full pluginId and not a pattern like you suggested.

@knutwannheden
Copy link
Contributor

Excellent! I wasn't sure if I was missing something here.

@sambsnyd sambsnyd added this to the 7.35.0 milestone Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants