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.

Sign up for GitHub

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

Jump to bottom

feat: Added option to comment out property in yaml #4741

Merged
merged 7 commits into from
Dec 5, 2024
Merged

feat: Added option to comment out property in yaml #4741

merged 7 commits into from
Dec 5, 2024

Conversation

ashakirin
Copy link
Contributor

@ashakirin ashakirin commented Dec 4, 2024 •
edited by timtebeek
Loading

@timtebeek timtebeek self-requested a review December 4, 2024 12:26
ashakirin and others added 6 commits December 5, 2024 10:31
@ashakirin @github-actions
Update rewrite-yaml/src/main/java/org/openrewrite/yaml/CommentOutProp…
3a05528 
…erty.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@ashakirin @github-actions
Update rewrite-yaml/src/main/java/org/openrewrite/yaml/CommentOutProp…
ca18bdf 
…erty.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Handle nullable Boolean correctly for usage from rewrite.yml
7c1def4
@timtebeek
Drop unused default
e1e3cb8
@timtebeek
Prevent double negation
7b2e8cb
@timtebeek
Adopt else if
cf08a9c
timtebeek
timtebeek approved these changes Dec 5, 2024
View reviewed changes
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.

Thanks for the addition and thorough tests! I've added some light polish to ensure any declarative usages do not fail with an NPE on the nullable field added.

@timtebeek timtebeek merged commit 20de532 into openrewrite:main Dec 5, 2024
2 checks passed
@timtebeek timtebeek mentioned this pull request Dec 5, 2024
Closed
@ashakirin
Copy link
Contributor Author

ashakirin commented Dec 6, 2024 •
edited
Loading

@timtebeek Thanks, Tim!
The only thing that concerns me a bit, is that recipes for normal properties and for yaml doing the same things, but have two different names and different defaults:

  • yaml: CommentOutProperty
  • properties: AddPropertyComment

Do you think would be possible to consolidate the names?
Andrei

@timtebeek
Copy link
Contributor

Sure! the yaml CommentOutProperty seems to predate the recent AddPropertyComment; would you be ok with seeing that later one renamed? As folks externally might have already incorporated that older recipe, whereas for the new one not as much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@timtebeek timtebeek timtebeek approved these changes

Assignees

@ashakirin ashakirin

Labels
None yet
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ashakirin @timtebeek