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

Remove BeanMethodsNotPublic from SpringBoot2BestPractices (again) #645

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

nmck257
Copy link
Collaborator

@nmck257 nmck257 commented Dec 5, 2024

That said, getting philosophical:
I think we typically have a design pattern where "upgrades" and "best practices" are kept separate, and "best practices" allows for more opinionated/noisy changes. That's what the JUnit 5 recipes look like, iirc.

This file violates that pattern by having the 2.0 upgrade recipe include the best practices recipe. Buuuut, it's been that way for over a year, and the other included recipes aren't nearly as disruptive as this one.

I could imagine a spectrum with one extreme having only the most-strictly necessary changes, and the other end having the most noisy and least-popularly-agreed-upon changes, and we slice that spectrum into sections like:

  • 1: "upgrades"
  • 2: "best practices"
  • 3: "standalone recipes"
  • 4: "not even recipes"

And in that model, maybe the other recipes in SpringBoot2BestPractices happen to fall closer to section 1, and BeanMethodsNotPublic is in section 3, and the composite recipe happens to be named for section 2 and then is included in the section 1 UpgradeSpringBoot_2_0 recipe. So it's easy for someone to think "this is a section 2 recipe, lemme add it to that bucket" and then it sneaks into section 1 and that feels bad. But it's all fuzzy.

@timtebeek
Copy link
Contributor

timtebeek commented Dec 5, 2024

Thanks for the nuanced discussion here @nmck257 ; sorry for any disruption this might have caused. It seems to have slipped in with

Would you be ok with moving that recipe to the Spring Boot 3.x best practices? That one seemed closer aligned with 2. above, and is not hooked into any of the upgrades by default. We similarly moved the virtual threads recipe there as well in c3a79c9, which is also opinionated and not a perfect fit everywhere.

The full 2.0 best practices recipe can move to src/main/resources/META-INF/rewrite/best-practices.yml as well, probably with a note to prevent another regression.

@timtebeek
Copy link
Contributor

timtebeek commented Dec 6, 2024

I've applied my suggestions from above; hope you agree! Figured with the note and stated goals this would now be clear.

@timtebeek timtebeek merged commit 7a5c362 into main Dec 6, 2024
1 check failed
@timtebeek timtebeek deleted the nmck257-patch-1 branch December 6, 2024 09:01
@nmck257
Copy link
Collaborator Author

nmck257 commented Dec 6, 2024

sorry for the delay -- yeah, no objections to your changes. thanks for moving this along!
I might draft a PR later that goes a step further to lift some of the existing SB2 "best practices" recipes up to the upgrade recipe itself, and remove "best practices" from the upgrade recipe; haven't decided myself yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants