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

Publish SyntaxRewriter.visitChildren #50

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@brentdax

brentdax commented Dec 4, 2018

There’s currently no good way to rewrite the children of a node while using SyntaxRewriter.visitAny(_:). This commit renames an existing internal method to visitChildren(of:) and makes it public so you can call it from a visitAny(_:) override.

This may not be the best way to address the issue; I'm open to alternatives. Or there might be some reason you're not supposed to do this at all, I suppose.

Publish SyntaxRewriter.visitChildren
There’s currently no good way to rewrite the children of a node while using SyntaxRewriter.visitAny(_:). This commit renames an existing internal method to visitChildren(of:) and makes it public.

@brentdax brentdax requested a review from harlanhaskins Dec 4, 2018

@harlanhaskins

This comment has been minimized.

Contributor

harlanhaskins commented Dec 4, 2018

The issue with this is you force the creation of the new node, from a potentially unverified child layout, onto the client.

swift-format solves this by just keeping an instance variable of the currently visited node, and returning nil from visitAny if the provided node is that node.

@brentdax

This comment has been minimized.

brentdax commented Dec 4, 2018

I could rework my code along those lines (actually, my visitAny(_:) is very similar to swift-format's, right down to the reduce), but if this is a common pattern, why is it so awkward to write?

I notice that both examples want to traverse depth-first, so one alternative might be to add a visitAnyPost(_:) or change visitPost(_:) to (optionally?) return a new value.

@brentdax brentdax closed this Dec 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment