Skip to content

✨ add parents to regexp visitor #87

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

Closed
wants to merge 1 commit into from

Conversation

connor4312
Copy link

Duplicating mysticatea#18

Copy link
Member

@MichaelDeBoey MichaelDeBoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@connor4312 Please rebase your branch onto latest main & resolve conflicts

@RunDevelopment
Copy link

Cool PR!

Question: Is it intentional that the parents array does not include the parents of the root node (= the node we call RegExpVisitor#visit on)? This seems strange to me, because it means that parents does not contain all ancestors of a node. In other words, I assumed !currentNode.parent || currentNode.parent == parents.at(-1) to be an invariant.

About the name "parents": a parent is typically the first direct ancestor of a node. So maybe we should name the parameter ancestors? I'm not sure which one is better, though.

Also, maybe we could do that better than Node[] for the type of parents? E.g. we know that the parents array of a Pattern node can only contain a RegExpLiteral node, and we know that the parents array of a quantifier can't contain a Character node. I think it would be nice if we had e.g. onGroupEnter?(node: Group, parents: readonly Ancestor<Group>[]): void (using the Ancestor type from here in this example).
I think this would make it easier for people to use the parents array, since TS has a better idea about its contents.

@ota-meshi
Copy link
Member

I don't think we need to add this parameter.
You can already easily access the parent node using node.parent.
What use cases are new parents useful for?

@connor4312
Copy link
Author

ah, you're right. Somehow missed that however many years ago I wrote this.

@connor4312 connor4312 closed this Jun 7, 2023
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.

4 participants