Format if elements. #1344
Merged
Format if elements. #1344
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
When I added support for constructors, I introduced a little API to let pieces enforce constraints between their state and the states of other child pieces. It worked but felt a little bolted on. In particular, enforcing the constraints during CodeWriter formatting meant we didn't prune invalid solutions as efficiently as we could. I'm working on if elements now and I needed a better way to handle constraints between pieces for that, so I went ahead and refactored the way this works to be, I think, generally better: - Enforce constraints between pieces during solution expansion instead of during formatting. This means that we eagerly discard any solution (and the whole tree of solutions that could have come from it) if it doesn't meet the constraints. - Remove the `_pieces` fields in PieceStateSet. With the previous optimization to track the next unsolved piece in Solution, that list wasn't doing much of anything. It's only use (aside from debug output) was for determining the order that pieces are compared when comparing two solutions. But we can get that from iterating over the _pieceStates map since maps in Dart track their key insertion order. - Update ConstructorPiece to use this new API.
I was hoping to be able to reuse more of the createIf() code in PieceFactory for both if elements and if statements, but there are enough differences between the two that that didn't work out, so I just inlined the code for both of them directly in AstNodeVisitor. I was able to reuse the existing IfPiece, though, which is nice.
natebosch
approved these changes
Dec 19, 2023
| return switch (node.expression) { | ||
| ListLiteral(:var elements, :var rightBracket) || | ||
| SetOrMapLiteral(:var elements, :var rightBracket) | ||
| when elements.canSplit(rightBracket) => |
There was a problem hiding this comment.
Consider adding a comment explaining why element.canSplit indicates a non-empty collection.
kallentu
approved these changes
Dec 25, 2023
(Forgot to save this change before committing. :( )
# Conflicts: # lib/src/back_end/solution.dart
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.
I was hoping to be able to reuse more of the createIf() code in PieceFactory for both if elements and if statements, but there are enough differences between the two that that didn't work out, so I just inlined the code for both of them directly in AstNodeVisitor.
I was able to reuse the existing IfPiece, though, which is nice.