Skip to content

Conversation

@mostafa
Copy link
Contributor

@mostafa mostafa commented Jul 10, 2025

Problem

The Remove operation used naive line-based removal that broke YAML structure for flow styles:

  • {a: valueA, b: valueB} => removing b would delete the entire line, corrupting the document
  • [item1, item2, item3] => removing item2 would delete the entire line

Solution

  1. Fixed yamlpath panic: Added missing node type handlers (block_mapping_pair, flow_pair, block_sequence_item) to Feature::kind()
  2. Better removal strategy: Analyzes parent container to determine appropriate removal method
  3. Flow style support: Extracts flow content, parses as YAML, removes target element, serializes back to flow style
  4. Preserves existing behavior: Block mappings/sequences still use line-based removal where appropriate

Testing

Added a few tests covering all YAML styles and some corner cases.

Known Limitations

Added two skipped tests to show how brittle nested structure detection is. This is a TODO for future.

xref #876

@mostafa mostafa marked this pull request as ready for review July 10, 2025 21:25
Comment on lines +453 to +469
// Check if the parent content contains a flow mapping or flow sequence
if parent_content.contains('{') && parent_content.contains('}') {
// This is a flow mapping (possibly within a block mapping)
handle_flow_mapping_removal_in_context(
document,
&parent_feature,
&patch.route,
content,
)
} else if parent_content.contains('[') && parent_content.contains(']') {
// This is a flow sequence (possibly within a block mapping)
handle_flow_sequence_removal_in_context(
document,
&parent_feature,
&patch.route,
content,
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned these kinds of checks aren't sound/won't generalize, e.g. here's a parent context that will pass the first branch here but is actually meant for the second:

{[
  foo: bar,
  baz: abc,
]}

(and same for flow mappings inside of flow arrays, e.g. [{foo: bar}])

Do you have any thoughts on how to generalize these? I don't have a great sense for what would work well here -- I think testing for individual tokens without knowing where they are in the syntax tree will unfortunately be too brittle 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a look tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to implement this using tree-sitter, but gave up because of complexity. Instead I added two skipped tests to do it in the future (in a separate PR).

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks! I'll have another look tomorrow.

@woodruffw woodruffw added enhancement New feature or request autofix Auto-fix functionality labels Jul 23, 2025
@woodruffw
Copy link
Member

(Just noting that I haven't forgotten about this, I've just been thinking about it -- the current yamlpatch ops are at the boundary of how much complexity I can hold in my head at once, so I've been thinking about larger architectural changes to yamlpath/new approaches entirely that would simplify our patching. Most of that is probably not worth the effort in the short-medium term, though.)

@mostafa
Copy link
Contributor Author

mostafa commented Jul 31, 2025

Thanks for the update. It is really mind-boggling. I wish we could completely move to tree-sitter for yamlpatch, but maybe in the next version.

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

Labels

autofix Auto-fix functionality enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants