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

Assigning commentBefore on a Pair #157

Closed
cmoesel opened this issue Apr 22, 2020 · 2 comments · Fixed by #161
Closed

Assigning commentBefore on a Pair #157

cmoesel opened this issue Apr 22, 2020 · 2 comments · Fixed by #161
Labels
bug Something isn't working

Comments

@cmoesel
Copy link

cmoesel commented Apr 22, 2020

I want to add a pair to my document with a comment before the pair, e.g.:

# This is a pair
foo: bar

I tried doing this using this code:

const pair = new Pair('foo', 'bar');
pair.commentBefore = ' This is a pair';
doc.add(pair);

But when I run the code, I get the following error:

(node:86824) UnhandledPromiseRejectionWarning: TypeError: Cannot create property 'commentBefore' on string 'foo'
    at Pair.set commentBefore [as commentBefore] (/path/to/my/project/node_modules/yaml/dist/schema/Pair.js:49:28)

As a more general question, how can I use YAML to create a document that has line comments preceding key/value pairs and/or line comments after a pair? E.g., what's the best way to do something like this:

# this is a comment about id before a pair
id: 123
# this is a comment about url before a pair
url: http://example.org/docs
# this is a comment after a pair that says you can optionally uncomment the next line
# debug: true

# this is a comment about author before a pair. I put this here to show that I
# can't use doc.comment to make that last comment (after a pair) work...
author: Joe Schmoe

using yaml 1.9.2

@eemeli eemeli added the bug Something isn't working label Apr 23, 2020
@eemeli
Copy link
Owner

eemeli commented Apr 23, 2020

In order to add a comment before a pair, you'll want to add that comment before the pair's key, i.e. the "foo" in your first example. That's actually what the pair.commentBefore assignment is trying to do internally, but it's not recognising that the key hasn't been wrapped in a Node; hence the somewhat unhelpful error.

Currently, the smallest change needed to do what you want would be something like this:

const pair = new Pair(YAML.createNode('foo'), 'bar')
pair.commentBefore = ' This is a pair'
doc.add(pair)

Now, clearly something needs to change in order to make that less painful. I'm just not sure what. Brainstorming, at least the following are possible:

  1. In new Pair:
    1. Always wrap at least the key in a Node
    2. Always wrap both key and value in Nodes
    3. Add a third arg wrapScalars to match that of YAML.createNode
  2. In Pair.commentBefore:
    1. If key is not a Node, silently wrap it in a node.
    2. If key is not a Node, throw a more helpful error.
    3. Always throw an error directing to use pair.key.commentBefore instead.
    4. Refactor to remove the get/set of the key's commentBefore, resolving them somehow differently.

Thoughts on the above are quite welcome.

@cmoesel
Copy link
Author

cmoesel commented Apr 23, 2020

Ah! I somehow missed that I can make each part of the Pair a node and control comments before or after each part. Perfect! As for making it a bit more streamlined... I just started using this library yesterday, so I'm not sure I have an educated opinion. For example, I don't know what the implications are of always wrapping the key or value in a node (is there a downside or unexpected behavior?).

I can tell you that as a first time user, the behavior wasn't quite what I expected for commentBefore or comment.

  • Based on (skimming) the documentation, I expected myPair.commentBefore to put a comment before the full pair (i.e., before the key) but as reported above, I got an error.
  • Based on (skimming) the documentation, I expected myPair.comment to put a comment after the full pair (i.e., after the value) but it actually put a comment between the key and the value (in the middle of the pair).

I expect that changing the behavior of comment to go after the full pair might affect existing users who now expect it to work the way it currently does -- so I don't know if you'd want to change that. But I thought I'd let you know anyway.

Regarding the original issue, I guess I'd say that as a user, I think better error messages with some direction on how to fix it would be a sufficient solution. An ideal solution would make it "just work" the way I tried, but only if it can be done without any negative side-effects.

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

Successfully merging a pull request may close this issue.

2 participants