-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
uast/nodes/node.go
Outdated
@@ -948,3 +948,28 @@ func UniqueKey(n Node) Comparable { | |||
return unkPtr(ptr) | |||
} | |||
} | |||
|
|||
// ChildrenCount returns a number of immediate children nodes. For arrays it will be the | |||
// length of an array. For objects, all child Object and Array nodes in fields will be counted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused (until I read the code) about whether this means the Array is counted as 1 (for itself) or for its children. May I suggest
// ChildrenCount reports the number of immediate children of n. If n is an Array this is
// the length of the array. If n is an object, each object or array in a field of n counts as
// one child.
or words to that effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the comment, PTAL.
@@ -182,7 +182,11 @@ func AsPosition(m nodes.Object) *Position { | |||
} | |||
|
|||
// PositionsOf returns a complete positions map for the given UAST node. | |||
func PositionsOf(m nodes.Object) Positions { | |||
func PositionsOf(n nodes.Node) Positions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment to document that this is empty for a non-Object. Previously it was clear from the type signature, but no longer—and intuitively it's not obvious that an array wouldn't also have positions (e.g., the start and end of its span). Not saying it should, but if I asked for the position of an arbitrary node I would be surprised that it should't work for an Array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -216,7 +220,11 @@ func RoleList(roles ...role.Role) nodes.Array { | |||
} | |||
|
|||
// RolesOf is a helper for getting node UAST roles (see KeyRoles). | |||
func RolesOf(m nodes.Object) role.Roles { | |||
func RolesOf(n nodes.Node) role.Roles { | |||
m, ok := n.(nodes.Object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As 185. Is there some reason for this relaxation more generally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason to relax this requirement is that all XPath query and any manual node manipulations will usually return a nodes.Node
, and the user have to type assert it each time to use those functions.
CI seems to be failing for some reason |
Signed-off-by: Denys Smirnov <[email protected]>
Signed-off-by: Denys Smirnov <[email protected]>
Signed-off-by: Denys Smirnov <[email protected]>
CI passes, fixed comments and added test cases. Ready for another round. |
@bzz PTAL |
Add a
ChildrenCount
helper (requested by https://github.com/bblfsh/client-go/issues/109) and relax argument constraints forPositionsOf
,RolesOf
andTokenOf
(see https://github.com/bblfsh/client-go/issues/100).Fixes https://github.com/bblfsh/client-go/issues/109.