-
Notifications
You must be signed in to change notification settings - Fork 83
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
Generate comments for deprecated properties #639
Generate comments for deprecated properties #639
Conversation
src/test/java/org/openrewrite/java/spring/internal/GeneratePropertiesMigratorConfiguration.java
Outdated
Show resolved
Hide resolved
String regex = "(?<!" + inlineComment + ")$"; | ||
ChangeSpringPropertyValue changeSpringPropertyValue = new ChangeSpringPropertyValue(key, inlineComment, regex, true, null); |
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.
Comments are usually modeled as Comment, not as part of the value. I worry that diverging from this would mean that on a next parse of the project, the text value and comment will be split, and another comment will get inserted. That's not great for folks running recipes repeatedly. Can we adjust this to model comments as Comments instead?
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.
Yes, this can be a problem indeed, model comments as Comment will be cleaner solution
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.
Turns out: we don't actually model trailing comments as Comment, but as part of the value 🤯
@ParameterizedTest
@ValueSource(strings = {"#", "!"})
void entryValueThenComment(String commentStyle) {
rewriteRun(
properties(
"""
key=value %s this is a comment
""".formatted(commentStyle),
spec -> spec.beforeRecipe(p -> {
assertThat(p.getContent().get(0))
.isInstanceOf(Properties.Entry.class)
.extracting(e -> ((Properties.Entry) e).getValue().getText())
.isEqualTo("value");
})
)
);
}
This fails with
org.opentest4j.AssertionFailedError:
expected: "value"
but was: "value # this is a comment"
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.
I won't make you jump through hoops; we'll need to sort that ourselves 😅 .
openrewrite/rewrite@569e733
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.
Yeah, but the question is still if inline comment is good idea in case of long reason string. Perhaps normal comment will be better in this case?
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.
Regular comment would be fine as well indeed. Saves us the trouble of changing the model.
Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.gradle.UpdateGradleWrapper?organizationId=T3BlblJld3JpdGU%3D#defaults=W3sibmFtZSI6ImFkZElmTWlzc2luZyIsInZhbHVlIjoiRmFsc2UifV0= Co-authored-by: Moderne <[email protected]>
) * Fix incorrect path computation for Spring API endpoints The current logic did not cover the case where a `RequestMapping` with a path was defined on the Controller class, and no paths were defined in the `*Mapping` at the method level. * Do not disable the FindApiEndpointsTest * Simplify FindApiEndpoints and SpringRequestMapping * Add for now failing unit test * AnnotationMatcher does not support wildcards --------- Co-authored-by: Tim te Beek <[email protected]>
…ut alternative Refs: openrewrite#634
…ropertiesTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ropertiesTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…roperties.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…roperties.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ropertiesTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…pertiesMigratorConfiguration.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Refactored the RenameBean recipe into a scanning recipe to allow renaming usages that occur outside the file where the bean is defined. * Add missing language hints * Remove unused `fromDeclaration` methods --------- Co-authored-by: Hudson, Ryan <[email protected]> Co-authored-by: Tim te Beek <[email protected]>
* Add MigrateSpringdocCommon recipe * Apply suggestions from bot Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix syntax messed up by bot * Add MigrateSpringdocCommon recipe * Apply suggestions from bot Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix syntax messed up by bot * Fix build fail on SpringBoot_1_5 * Fix rebase error * Move to Spring Boot 2.6 tests, to match inclusion in `spring-boot-26.yml` --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tim te Beek <[email protected]>
…d declaration (openrewrite#631) * Add ChangeMethodParameter for modify parameters in method declaration Signed-off-by: Kun Chang <[email protected]> * Minor polish * Add correct year * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Add UpgradeSkipPolicyParameterType for Spring Batch Signed-off-by: Kun Chang <[email protected]> * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Fix tests * Also show ability to change interface methods --------- Signed-off-by: Kun Chang <[email protected]> Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
72e9657
to
ab34140
Compare
assertThat(((Yaml.Mapping) file.getDocuments().get(0).getBlock()).getEntries().get(1).getPrefix()) | ||
.isEqualTo(" # my comment\n")) |
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.
Add this validation that even though for .properties
we do not model Comments correctly, we should still model the comment for .yml
correctly to avoid repeated addition of the same comment.
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.
I am going to submit "clean" recipes to add comment into rewrite-properties and rewrite-yaml to have an option to add normal comment, not inline one
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.
Appreciate it! We can switch to those if needed; I stumbled across the one that comments out entries which I figured was good enough for an initial version, but happy to make the change to retain the property but mere add a comment.
6ddcbff
to
3a8ff2a
Compare
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 this come together; thanks a lot @ashakirin ! Hope you don't mind that I've made quite some changes as well, and regenerated the property migrations to use your new recipe. Neat to finally clear those out for as much as they might still have been present.
Generated comment recipes for deprecated properties without replacement