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

MethodNameCasing poorly implemented #4180

Open
delanym opened this issue May 9, 2024 · 2 comments
Open

MethodNameCasing poorly implemented #4180

delanym opened this issue May 9, 2024 · 2 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation

Comments

@delanym
Copy link

delanym commented May 9, 2024

Essentially the implementation of the MethodNameCasing recipe is inadequate.
All the documentation https://docs.openrewrite.org/recipes/staticanalysis/methodnamecasing
can tell me is Method names should comply with a naming convention.
Well what is that convention and how do I change it? There are no options for that.

A tag is listed: https://sonarsource.github.io/rspec/#/rspec/S100
Which includes the text For example, with the default provided regular expression ^([A-Z0-9_]*|[a-z0-9_]*)$, the method...

If this was in fact the default regular expression then why did it convert a method name get_CHAR_NullableOutputList to getCHARNullableOutputList?

A further issue is that it did not convert the method name get_LongInputList since there was already a getLongInputList method, and yet it did not cancel the recipe with an error/warning.

I'm logging this as a bug not a feature because it applied the recipe and broke the build.

@delanym delanym added the bug Something isn't working label May 9, 2024
@timtebeek timtebeek added the documentation Improvements or additions to documentation label May 9, 2024
@timtebeek
Copy link
Contributor

Thanks for the feedback! I can understand the confusion; up to recently that recipe linked to an older source at Sonar that's not been maintained; they'd pointed that out and we've since updated the links.

The old link was a little more descriptive in what is and is not compliant, in particular using a Java example: https://rules.sonarsource.com/java/RSPEC-100/

Shared naming conventions allow teams to collaborate efficiently.

This rule raises an issue when a method name does not match a provided regular expression.

For example, with the default provided regular expression ^[a-z][a-zA-Z0-9]*$, the method:

public int DoSomething(){...} // Noncompliant

should be renamed to

public int doSomething(){...}

Exceptions
Overriding methods are excluded.

@Override
public int Do_Something(){...} // Compliant by exception

I guess it would help to update our recipe description now that the external source isn't helping understand the changes made. Perhaps working back from the tests we have to document the changes.
https://github.com/openrewrite/rewrite-static-analysis/blob/1ed9c44b1413a388a7d436e8a7ca5bcacc40fc94/src/test/java/org/openrewrite/staticanalysis/MethodNameCasingTest.java#L85-L108

@mike-solomon is this one you could take a look at to update the displayed name and description?
https://github.com/openrewrite/rewrite-static-analysis/blob/1ed9c44b1413a388a7d436e8a7ca5bcacc40fc94/src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java#L58-L64

Beyond that we can then see how what the recipe does might conflict with what you were expecting @delanym ; how did things break specifically? A before/after example would be best, just to be sure we fix the bug as well that prompted you to log it here.

mike-solomon added a commit to openrewrite/rewrite-static-analysis that referenced this issue May 10, 2024
To hopefully be clearer as to what it does. Based on feedback from
the following issue: openrewrite/rewrite#4180
timtebeek added a commit to openrewrite/rewrite-static-analysis that referenced this issue May 10, 2024
* Adjusts MethdNameCasing name and description

To hopefully be clearer as to what it does. Based on feedback from
the following issue: openrewrite/rewrite#4180

* Update src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java

Co-authored-by: Tim te Beek <tim@moderne.io>

---------

Co-authored-by: timo-abele <mike-solomon@users.noreply.github.com>
Co-authored-by: Tim te Beek <tim@moderne.io>
@timtebeek
Copy link
Contributor

Mike was kind enough to update the display name and description already in

Would you feel we need to make further adjustments still? Perhaps an example with a before/after as seen in our tests would be best for anything not correctly handled right now. Making the examples runnable in that sense also helps rule out any outside influences, like Lombok leading to missing type information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
Status: Backlog
Development

No branches or pull requests

2 participants