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

Introduce MapKeyNode interface to limit node types for map key #312

Merged
merged 1 commit into from Aug 22, 2022

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Aug 18, 2022

This pull request introduces ast.MapKeyNode interface for the following reasons.

  • There are only limited node types for map keys. For example MappingValueNode never allowed as a map key. But current AST allows to create such unexpected maps. This PR changes the argument key of ast.MappingValue to ast.MapKeyNode to prohibit creating unexpected maps.
  • In the ast package, stringWithoutComment() string is implemented for all node types, but this method is only required for map key types. CommentNode having stringWithoutComment looks very weird. So I removed all the unnecessary stringWithoutComment implementations.

For ast package users, following is the complete list of the changes.

  • New interface ast.MapKeyNode is introduced. All the node types implementing ast.ScalarNode and ast.MergeKeyNode, ast.MappingKeyNode satisfy ast.MapKeyNode. Note that ast.LiteralNode never appears as map key but is a scalar node thus satisfies ast.MapKeyNode.
  • ast.Null, ast.Bool, ast.Integer, ast.Float now return the concrete types instead of ast.Node (Ref: The implementing package should return concrete (usually pointer or struct) types).
  • ast.MappingValue now accepts key ast.MapKeyNode instead of ast.Node. Also, func (*MapNodeIter) Key() now returns ast.MapKeyNode instead of ast.Node.

@itchyny itchyny changed the title Introduce MapKeyNode to limit node types for map key Introduce MapKeyNode interface to limit node types for map key Aug 18, 2022
@goccy
Copy link
Owner

goccy commented Aug 22, 2022

Thank you for the great PR !!! ast.MapKeyNode is very nice ! LGTM 😄

@goccy goccy merged commit 03bdafe into goccy:master Aug 22, 2022
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.

None yet

2 participants