yaml icon indicating copy to clipboard operation
yaml copied to clipboard

CST.setScalarValue() does not preserve non-semantic indentation for block scalars

Open pjeby opened this issue 2 years ago • 3 comments

Describe the bug CST.setScalarValue() does not preserve non-semantic indentation for block scalars

To Reproduce Apply setScalarValue() to change the content of a 4-space indented block scalar value in an unindented map. The indentation will be reduced to 2 spaces if afterKey is set, zero otherwise.

Expected behaviour The original indentation should be kept.

Versions (please complete the following information):

  • Environment: node 14.16 / electron 13.5.2 / chrome 91.0.4472.164
  • yaml: 2.0.0-10

Additional context I'm not sure if this is technically a problem with setScalarValue, or with parsing not keeping track of the original indentation relative to the key.

pjeby avatar Jan 16 '22 23:01 pjeby

Yeah, this is a bug with CST.setScalarValue(). What should happen is that it looks at the previous raw content of the node, and figures out a suitable content indent from that, somewhere around here: https://github.com/eemeli/yaml/blob/7998a957d1b9cecffc0a79f6023c6882e073d6eb/src/parse/cst-scalar.ts#L157-L158

A PR for this would be very welcome, as I'm not really sure when I'll get to this myself.

eemeli avatar Jan 17 '22 07:01 eemeli

Hm. Would it perhaps be better to just capture the indent at parse time, though? I mean, the issue is that the node's .indent is 0 in my use case, rather than the first-line indent of the block scalar.

pjeby avatar Jan 17 '22 09:01 pjeby

Determining the scalar content indent is normally done during composition, i.e. when the actual node value is parsed from the CST contents; here. When working with CST nodes directly, that is called internally by CST.resolveAsScalar().

So the issue here is that for this use, part of that work needs to also be done in CST.setScalarValue(), and that the logic for that needs to get extracted from its current wrappings.

eemeli avatar Jan 17 '22 11:01 eemeli