Add iterator for ASTNodeArray

[Xapphire13]

No description provided.

[maxbrunsfeld]

Great addition! FWIW, I think the most efficient way to implement this will be to use nextSibling repeatedly.

[Xapphire13]

@maxbrunsfeld, turns out it was a smaller change than I anticipated =].

Additionally, my testing thus far has been manual. Is adding JS tests on the roadmap for this? If so, which test framework (Just in case I make another change before tests are added)?

[maxbrunsfeld]

Great! Thanks for doing the optimization. Left two minor comments.

[maxbrunsfeld]

I think we shouldn't document this property; let's just treat it as an implementation detail of node arrays.

[Xapphire13]

I thought that too, I was just concerned about someone seeing it later down the line and thinking "Hey, this isn't documented, must be a bug". If only JS had true private methods =]

[maxbrunsfeld]

Even though it will mean more duplication, I'd prefer to have one conditional check at the beginning of the loop, rather than a closure with a conditional on each iteration. Something like:

if (this.isNamed) {
  while ((node = node.nextNamedSibling)) {
    yield node
  }
} else {
  while ((node = node.nextSibling)) {
    yield node
  }
}

[Xapphire13]

I can do this, but I wanted to mention that the way it's currently implemented doesn't use a closure and the this.isNamed is only evaluated once. The only difference this change would make would be removing the stack frame of the lambda on lines 31/32.

[maxbrunsfeld]

Ah, thanks for pointing that out; I misread your code.

[maxbrunsfeld]

Additionally, my testing thus far has been manual. Is adding JS tests on the roadmap for this?

Yeah, the test suite lives in tree-sitter-cli. It's a bit odd, but I do all of the testing in that repo because that's the repo that allows you to create grammars.

If you're up for adding some tests for this new code in node_test.js after this PR is merged, that'd be 💯.