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

Chained AssertJ assertions should be simplified to the corresponding dedicated assertion #556

Merged
merged 18 commits into from
Aug 11, 2024

Conversation

velo
Copy link
Contributor

@velo velo commented Jul 25, 2024

This is not ready yet, but working on it.

From time to time, I get toasted by a sonar code smell, and I decided to make something about it
https://next.sonarqube.com/sonarqube/coding_rules?open=java%3AS5838&rule_key=java%3AS5838

What's changed?

For now, created a test for all rules and start implementing missing recipes.

For now, I'm creating a PR to ask questions.

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@timtebeek timtebeek self-requested a review July 25, 2024 06:13
github-actions[bot]

This comment was marked as outdated.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Great work here @velo! I like how you went ahead and got a comprehensive view of what is and isn't covered already through those parameterized tests. It looks like we already pass ~2/3 of the tests here.

image

With some adjustments I think we can cover a few more common cases, before perhaps delegating the others to a follow up PR. Let me know your thoughts & plans here! :)

@timtebeek timtebeek added enhancement New feature or request recipe Recipe request labels Jul 25, 2024
@timtebeek timtebeek marked this pull request as draft July 25, 2024 09:34
@velo
Copy link
Contributor Author

velo commented Jul 25, 2024

I will get them sorted

@timtebeek
Copy link
Contributor

Hi @velo, are you ok with me polishing this up and merging a smaller increment already? I'm thinking we comment out the failing cases and merge the already nice improvements on this branch.

@velo velo marked this pull request as ready for review August 11, 2024 12:05
@velo
Copy link
Contributor Author

velo commented Aug 11, 2024

Hi @timtebeek

Sorry, I totally reduced the score of the change to make it a bite size change meant to be merged, but somehow forgot to write an update!

@velo
Copy link
Contributor Author

velo commented Aug 11, 2024

Hi @timtebeek

Sorry, I totally reduced the score of the change to make it a bite size change meant to be merged, but somehow forgot to write an update!

Go for it

@timtebeek timtebeek self-requested a review August 11, 2024 17:11
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

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

Nice improvements here! Like how we can cover more cases with less classes; Should help extend the current offerings. I've renamed & revised the classes a bit to make it clear when to use which. Hope you agree & thanks again!

@timtebeek timtebeek merged commit 8a037e8 into openrewrite:main Aug 11, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request recipe Recipe request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants