-
Notifications
You must be signed in to change notification settings - Fork 348
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 initial support for java references in application properties file #4697
Conversation
rewrite-properties/src/test/java/org/openrewrite/properties/trait/PropertiesReferenceTest.java
Outdated
Show resolved
Hide resolved
rewrite-properties/src/test/java/org/openrewrite/properties/trait/PropertiesReferenceTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
rewrite-properties/src/test/java/org/openrewrite/properties/trait/PropertiesReferenceTest.java
Outdated
Show resolved
Hide resolved
rewrite-properties/src/main/java/org/openrewrite/properties/trait/PropertiesReference.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
rewrite-properties/src/main/java/org/openrewrite/properties/trait/PropertiesReference.java
Outdated
Show resolved
Hide resolved
…ait/PropertiesReference.java Co-authored-by: Laurens Westerlaken <[email protected]>
rewrite-properties/src/test/java/org/openrewrite/properties/trait/PropertiesReferenceTest.java
Outdated
Show resolved
Hide resolved
rewrite-properties/src/main/java/org/openrewrite/properties/tree/Properties.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see! Approved already, but it's still marked as draft?
A quick glance from @knutwannheden would be appreciated, but not a hard requirement. Should we mark this as Incubating still?
That might be wise, the code be considered incubating, as most of it implements interfaces that are marked as incubation. I added the incubation annotation. |
I still think since the |
@Laurens-W I have no preference either way, and it's unclear what the convention is within this project. The |
What's changed?
Add initial support for generating java type references from a
application.*.properties
file, support has only been added for the values, not the keys.What's your motivation?
Adding this feature allows existing recipes that support references to be applied to
.properties
files. For more details see:Reference
to include.properties
and.yml
#4681Anything in particular you'd like reviewers to focus on?
The current implementation is still rather limited in its functionality and many design choices are still to be made.
Currently, references of
Kind.Type
orKind.Package
are generated for either a Java package, e.g.,com.openrewrite
, or a fully qualified type, such asjava.lang.Integer
, but notInteger
, using a regex derived from the current implementation ofSpringReference
. This matching is only done for the values in a properties file, not the keys. If this is not the case, nearly every key would likely qualify as a package since they usually contain at least a single ".". Is this as desired?I have added support for renaming
Properties.Entry
in theTypeMatcher
andPackageMatcher
and added a test forChangeType
andChangePackage
recipes. It would perhaps be nice to have a list of all the classes which need to support properties, e.g.AnnotationMatcher
?Anyone you would like to review specifically?
@timtebeek @Laurens-W
Have you considered any alternatives or workarounds?
Any additional context
Checklist