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

fix(document): add missing type #27

Merged
merged 2 commits into from
Aug 3, 2018

Conversation

ikatyang
Copy link
Contributor

@ikatyang ikatyang commented Aug 3, 2018

To be consistent with other AST nodes.

@eemeli
Copy link
Owner

eemeli commented Aug 3, 2018

This is actually not quite as clear as you might think. The type of an AST node is only set during parsing, and only if the keepNodeTypes option is true (as it is by default). When new Map, Seq and Scalar objects are constructed, they don't have a type set initially; this means that during stringification they use a default variant. Mappings & sequences are currently hard-coded to use block collections, and for scalars it depends on the schema; string values fall back to strOptions.defaultType which is PLAIN by default.

Setting that default value for type during construction is difficult in particular for scalars, because it'd mean that the Scalar constructor would need to be aware of the schema, and have a means of looking up the appropriate default type for the initial value. And if the scalar value is changed later to be of a different type, a subsequent stringification would use the previous value's default type, which would be surprising.

Furthermore, it's debatable whether the Document should be presented as an AST node, given that it contains quite a bit of functionality, metadata and internal references. Here's how I've put it in the documentation:

After parsing, the contents value of each YAML.Document is the root of an Abstract Syntax Tree of nodes representing the document (or null for an empty document).

If you still think that it'd be beneficial to include a type for the Document, then for symmetry that should be set in its parse() method, and only if keepNodeTypes is true. Btw, the reason why it's an option in the first place is that setting it to false allows for e.g. a YAML.parseDocument(src).toString() call to be used to normalise the data types to their default forms.

@ikatyang
Copy link
Contributor Author

ikatyang commented Aug 3, 2018

Thanks for the detailed explanation!

Yeah, I did notice that it might not be considered as an AST node from the implementation, though it still looks like an AST node (they shared the same properties: comment, commentBefore, range), but I didn't notice that the type in Document could be used in stringify like other nodes.

The biggest benefit for me is that I can easily identify those AST node-like objects by looking at the type property, since there's an option keepNodeTypes can control this behavior so I think it should be fine.

@eemeli eemeli merged commit 3b962c9 into eemeli:master Aug 3, 2018
@ikatyang ikatyang deleted the fix/ast-document-type branch August 4, 2018 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants