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

Improve MergeYaml recipe #5036

Merged
merged 12 commits into from
Feb 18, 2025
Merged

Improve MergeYaml recipe #5036

merged 12 commits into from
Feb 18, 2025
+994 −118

Conversation

jevanlingen
Copy link
Contributor

@jevanlingen jevanlingen commented Feb 13, 2025 •
edited
Loading

What's changed?

  • Support comments for the insert before option
  • Support insert before for sequences
  • Add support to insert after.

What's your motivation?

was written a little to fast. Lot's of edge cases were missing.

Any additional context

There are still bugs left:

  • Adding comments in the YAML snippet option does not really work; all cases where it does work are just by luck.
  • Comments in sequences do not work very well either. To fix this with current LST model, the same kind of code written inside the mergeMapping function should be written inside the mergeSequence too.

@jevanlingen jevanlingen requested a review from sambsnyd February 13, 2025 16:19
@jevanlingen jevanlingen self-assigned this Feb 13, 2025
@jevanlingen jevanlingen added bug Something isn't working recipe Requested Recipe labels Feb 13, 2025
github-actions[bot]

This comment was marked as off-topic.

Sign in to view

github-actions[bot]

This comment was marked as off-topic.

Sign in to view

github-actions[bot]

This comment was marked as off-topic.

Sign in to view

@jevanlingen jevanlingen mentioned this pull request Feb 14, 2025
Merged
3 tasks
@Jenson3210 Jenson3210 mentioned this pull request Feb 14, 2025
Draft
3 tasks
greg-at-moderne
greg-at-moderne approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@greg-at-moderne greg-at-moderne left a comment

Choose a reason for hiding this comment

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

LGTM, didn't read the test cases.

rewrite-core/src/main/java/org/openrewrite/internal/ListUtils.java Show resolved Hide resolved
rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYaml.java Show resolved Hide resolved
@timtebeek timtebeek mentioned this pull request Feb 17, 2025
Closed
github-actions[bot]

This comment was marked as off-topic.

Sign in to view

github-actions[bot]

This comment was marked as off-topic.

Sign in to view

timtebeek
timtebeek approved these changes Feb 17, 2025
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.

Approved apart from the earlier comments. Thanks again! Great to see the quick turnaround on this.

github-actions[bot]

This comment was marked as off-topic.

Sign in to view

@jevanlingen jevanlingen merged commit 2c6cfda into main Feb 18, 2025
2 checks passed
@jevanlingen jevanlingen deleted the improve-merge-yaml-with-comment-support-on-insert-before-option branch February 18, 2025 12:11
This was referenced Feb 18, 2025
Merged
Merged
Merged
Merged
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

@greg-at-moderne greg-at-moderne greg-at-moderne approved these changes

@sambsnyd sambsnyd Awaiting requested review from sambsnyd

Assignees

@jevanlingen jevanlingen

Labels
bug Something isn't working recipe Requested Recipe
Projects
OpenRewrite
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jevanlingen @timtebeek @greg-at-moderne