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

Adjust MethodNameCasing to work with C# #397

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Laurens-W
Copy link
Contributor

What's changed?

Introduce interfacing and factory pattern to dynamically retrieve a NamingConvention to be applied for given sourcefile

What's your motivation?

Introduction of C# as a supported language

Anyone you would like to review specifically?

@timtebeek @OlegDokuka @knutwannheden

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

@Laurens-W Laurens-W self-assigned this Nov 27, 2024
@Laurens-W Laurens-W added enhancement New feature or request recipe labels Nov 27, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/MethodNameCasing.java
    • lines 44-43

@knutwannheden
Copy link
Contributor

Possibly we should leverage the service mechanism we have in JavaSourceFile. We should pull that up API to SourceFile anyway one of these days. Just a thought.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Laurens-W
Copy link
Contributor Author

Possibly we should leverage the service mechanism we have in JavaSourceFile. We should pull that up API to SourceFile anyway one of these days. Just a thought.

It actually already is, so the generic interface could be called something like NamingService and every language can provide it's own implementation dynamically through whichever class implements SourceFile or a more specific variant of that

}

String toName = standardized.toString();
NamingService service = getCursor().firstEnclosing(SourceFile.class).service(NamingService.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

The JavaVisitor in fact has a service() method which is much easier to use. We might want to pull that up to TreeVisitor at some point.

We might also have to cache the services a bit more as "breadcrumbs" along the cursor path, so that the service lookups don't get too expensive for deeply nested elements. But that is also something for another day.

@Laurens-W Laurens-W changed the title Adjust MethodNameCasing to work with C# Adjust MethodNameCasing to work with C# Nov 28, 2024
Also move checks to NamingService
@Laurens-W Laurens-W marked this pull request as ready for review November 28, 2024 15:16
@Laurens-W Laurens-W marked this pull request as draft November 28, 2024 15:17
Comment on lines +106 to 108
String toName = service.standardizeMethodName(normalized);
if (!StringUtils.isBlank(toName) && !StringUtils.isNumeric(toName) &&
!methodExists(method.getMethodType(), toName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

standardizeMethodName is not guaranteed to give back a different method name; do we already have a test to check expecting no changes? Or should we add a comparison here between the old and new name?

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.

Approved already, but needs a rerun of the tests after this PR is merged:

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
Projects
Status: Ready to Review
Development

Successfully merging this pull request may close these issues.

3 participants