Revamp the PieceWriter API some. #1298
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current API is pretty confusing around when you should use `pop()`, `split()`, or both. If you get it wrong, sometimes it silently works but may leave a bug for later, or sometimes it fails it seemingly unrelated ways. Also, it's not clear whether you should call `pop()` first or `split()`. (It actually didn't matter.) I tried to simplify the API some. It's still more subtle and brittle than I would like, but it's not clear to me if the complexity is essential. I'm trying to build Piece trees that don't have unnecessary nodes, so some slippage between AST node boundaries and Piece tree boundaries is essential. I hope this API is better. The changes are: - Make `split()` implicitly `pop()` the previous piece. This means you never need to call both `split()` and `pop()`. Just call `split()`. You do still have to know when to prefer `pop()` instead of `split()`, but I added some documentation to try to clarify that. - Rename the main PieceWriter field from `writer` to `pieces`. I think this reads a little better at the callsites and makes it clearer what is being pushed, popped, and split. "Writer" doesn't really mean much. - Rename `space()` to `writeSpace()`. Then add a helper method in PieceFactory called `space()`. This way, almost all calls on `pieces` deal with pieces and calls that recurse into or write pieces of an AST are bare function calls: `token()`, `visit()`, and `space()`.
natebosch
reviewed
Oct 27, 2023
Co-authored-by: Nate Bosch <nbosch@google.com>
natebosch
approved these changes
Oct 30, 2023
kallentu
approved these changes
Oct 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current API is pretty confusing around when you should use
pop(),split(), or both. If you get it wrong, sometimes it silently works but may leave a bug for later, or sometimes it fails it seemingly unrelated ways. Also, it's not clear whether you should callpop()first orsplit(). (It actually didn't matter.)I tried to simplify the API some. It's still more subtle and brittle than I would like, but it's not clear to me if the complexity is essential. I'm trying to build Piece trees that don't have unnecessary nodes, so some slippage between AST node boundaries and Piece tree boundaries is essential.
I hope this API is better. The changes are:
Make
split()implicitlypop()the previous piece. This means you never need to call bothsplit()andpop(). Just callsplit(). You do still have to know when to preferpop()instead ofsplit(), but I added some documentation to try to clarify that.Rename the main PieceWriter field from
writertopieces. I think this reads a little better at the callsites and makes it clearer what is being pushed, popped, and split. "Writer" doesn't really mean much.Rename
space()towriteSpace(). Then add a helper method in PieceFactory calledspace(). This way, almost all calls onpiecesdeal with pieces and calls that recurse into or write pieces of an AST are bare function calls:token(),visit(), andspace().