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 trait to identify java package references in xml #4587

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

Conversation

Laurens-W
Copy link
Contributor

Anyone you would like to review specifically?

@timtebeek

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 Oct 17, 2024
@Laurens-W Laurens-W added the enhancement New feature or request label Oct 17, 2024

public static class Matcher extends SimpleTraitMatcher<ContainsPackageReference> {
private final Pattern PACKAGE_OR_TYPE_REFERENCE = Pattern.compile("^([a-zA-Z_][a-zA-Z0-9_]*)(\\.[a-zA-Z_][a-zA-Z0-9_]*)*(\\.[A-Z][a-zA-Z0-9_]*|\\$[A-Z][a-zA-Z0-9_]*)*(\\.\\*)?$");
private final List<String> ATTRIBUTES_THAT_REFERENCE_PACKAGE_OR_TYPE = Arrays.asList("class", "type");
Copy link
Contributor Author

@Laurens-W Laurens-W Oct 17, 2024

Choose a reason for hiding this comment

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

Obviously these aren't all attributes and elements that could contain a package reference, these lists need to be carefully expanded

Copy link
Contributor

@timtebeek timtebeek Oct 18, 2024

Choose a reason for hiding this comment

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

I think regular expressions are an unsafe way to go about it, but I'm not exactly sure what a better approach would be. XPath would be a little more predictable, but also feel out of place here to have Spring/Micronaut/Quarkus/EE expressions all in here to be picked up when changing types.

Perhaps @knutwannheden has some suggestions on using service discovery where frameworks specific recipe modules can provide a mechanism to provide additional paths to check and convert.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would probably be best if we could register some kind of "extensions" with parsers like those for XML, YAML, Properties, etc. So we could then already while ingesting the source file attach some kind of JavaTypeReference marker to the LST elements. Then the ChangeType recipe could check if the SourceFile if it has some kind of HasJavaTypeReferences marker or possibly it even uses the TypesInUse we use for JavaSourceFile.

An additional complication may be that these "parser extensions" might have to be registered dynamically (e.g. depending on whether the project is a Spring project or not).

This could quickly end up being quite a bit of work. But I get the impression that this would long-term be the better and more sustainable solution. Maybe we can still do something like this, but start out small?

Laurens-W and others added 3 commits October 24, 2024 17:20
…eReferences.java

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

Choose a reason for hiding this comment

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

This should be moved to rewrite-spring


public static JavaTypeReferences build(Xml.Document doc) {
Set<JavaTypeReference> typeReferences = new HashSet<>();
new SpringJavaTypeReference.Matcher().asVisitor(reference -> {
Copy link
Contributor Author

@Laurens-W Laurens-W Oct 25, 2024

Choose a reason for hiding this comment

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

Need to figure out a way to dynamically pass a trait to use here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants