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

Inconsistent comments handling for empty nodes #491

Open
roman-burdeiniy opened this issue Aug 9, 2023 · 4 comments
Open

Inconsistent comments handling for empty nodes #491

roman-burdeiniy opened this issue Aug 9, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@roman-burdeiniy
Copy link

roman-burdeiniy commented Aug 9, 2023

Describe the bug
Inconsistent behaviour of parseDocument function.
Once yaml contains deep hierarchical of empty nodes the comment from the next node on a top level is getting ignored. Instead it appears on a wrong level of hierarchy. See example below.

To Reproduce
Consider yaml

SettingsMenuPopup:

  #@desc: Colours for settings button by modes.
  iconColor:
    open:

  #@desc: Settings button configuration.
  #@class: Button
  button:

Use const document = yaml.parseDocument(fileSource, { sortMapEntries: true });

Expected behaviour
The comment "@desc: Settings button configuration.\n@class: Button" should be on "button" Scalar on level:
document.contents.items[0].value.items[1].value.comment.

Screenshot 2023-08-09 at 10 03 25

Actual behaviour
The comment "@desc: Settings button configuration.\n@class: Button" appears on "iconColor.open" level Scalar:
document.contents.items[0].value.items[0].value.items[0].value.comment.

Screenshot 2023-08-09 at 10 12 53

Versions (please complete the following information):

  • Environment: [e.g. Node.js 14.7.0 or Chrome 87.0]
    Node v20.0.0
  • yaml: [e.g. 1.10.0 or 2.0.0-2]
    2.3.1
    According to my local tests the bug was introduced in 2.0.0-4

Additional context
The issue will be solved if I explicitly apply null to empty node in yaml, e.g.

SettingsMenuPopup:

  #@desc: Colours for settings button by modes.
  iconColor:
    open: null

  #@desc: Settings button configuration.
  #@class: Button
  button:
@roman-burdeiniy roman-burdeiniy added the bug Something isn't working label Aug 9, 2023
@eemeli
Copy link
Owner

eemeli commented Aug 9, 2023

This is pretty much the same as #490. As I mention there:

YAML does not specify how comments are attached, and as you see here, there isn't always just one way to attach a comment. You're welcome to suggest changes in the logic for comment attachment, but you'll probably need to submit a PR yourself. Be warned, though, the heuristics on this can get a bit hairy as it's pretty much all corner cases.

@roman-burdeiniy
Copy link
Author

roman-burdeiniy commented Aug 10, 2023

yeah, thanks for answering and seems it is the same issue as 490 as you said. The comments in yaml can be practically anywhere and they are not bound in any way to the scalar except visually.
But yaml lib actually tries to do this and I found this super useful in our project use cases (we probably should not relay on comments in our build process though, but unfortunately not possible to get rid of this now). Anyways my point is that in order to keep parsing comments per node yaml lib should define strict rules of attaching a comment. Other "free style comments" could be ignored. Scanning for a comment all around and trying to guess to what it belongs to will probably never be completely reliable.

@eemeli
Copy link
Owner

eemeli commented Aug 10, 2023

I would reiterate that you're welcome to suggest changes in the logic for comment attachment, but you'll probably need to submit a PR yourself.

@roman-burdeiniy
Copy link
Author

roman-burdeiniy commented Aug 11, 2023

I see, thank you for opportunity to contribute. But unfortunately I'm not able to work on third party projects right now. What I wanted to figure out is if this is considered as a bug and if yes then if it is planned to be fixed by active contributors. Otherwise we would probably just revert to previous stable version as of now

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