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.

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

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

Open
pjeby opened this issue Jan 16, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@pjeby
Copy link

pjeby commented Jan 16, 2022

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 pjeby added the bug Something isn't working label Jan 16, 2022
@eemeli
Copy link
Owner

eemeli commented Jan 17, 2022

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:

let indent = 'indent' in token ? token.indent : null
if (afterKey && typeof indent === 'number') indent += 2

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

@pjeby
Copy link
Author

pjeby commented Jan 17, 2022

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.

@eemeli
Copy link
Owner

eemeli commented Jan 17, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants