-
Notifications
You must be signed in to change notification settings - Fork 356
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
fix: MergeYaml
block indentation not as expected
#4358
Conversation
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
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.
Some suggestions could not be made:
- rewrite-gradle/src/main/groovy/RewriteSettings.groovy
- lines 31-36
I merged main into that WIP branch, but I did it in the web interface and it looks like I misplaced a bit of syntax so it isn't compiling anymore. Whoops 😅 |
6201fbc
to
bdc90bf
Compare
rewrite-yaml/src/main/java/org/openrewrite/yaml/MultilineScalarAdded.java
Outdated
Show resolved
Hide resolved
@sambsnyd I cleaned it up by going back to main and applying all the changes from another working branch ( |
rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java
Outdated
Show resolved
Hide resolved
* Added test cases to check inline comment retention. * Updated issue number to check inline comment retention. --------- Co-authored-by: j0m17kw <[email protected]> Co-authored-by: Peter Streef <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
33e702b
to
9b8c2af
Compare
rewrite-yaml/src/main/java/org/openrewrite/yaml/format/IndentsVisitor.java
Outdated
Show resolved
Hide resolved
rewrite-yaml/src/main/java/org/openrewrite/yaml/format/IndentsVisitor.java
Outdated
Show resolved
Hide resolved
rewrite-yaml/src/main/java/org/openrewrite/yaml/format/IndentsVisitor.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 with three minor suggestions/questions. Merge once those are resolved.
I think I have identified a bug in the yaml parser + writer.
when we parse the following yaml:
the value of the script mapping is:
when writing this yaml it correctly writes back as the original because of this.
However, when we want to insert this yaml somewhere into another document the prefix is not taken into account. So if I were to insert
into the same spot I end up with:
I think we should remove the whitespace from the block values and use the prefix of the parent (in case of a LITERAL or FOLDER style scalar) to indent when printing
It might be better to add the current indentation to the printer context.