Couple of suggestions & a major change - #21
Open
vnagendra wants to merge 5 commits into
Open
Conversation
Added break for the pipe operator when the lhs is not just a value ex: `.a as X |`, and added breaks for `if/elif/else` operator. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… takes inspiration from Rob Pike's talk around writing a scanner in go. That's what was happening before as well. There wasn't anything wrong with the original idea. I just reimplemented it because it seemed simpler to think of the problem this way rather than the way it was written. Code reduction/increase is a consequence of reframing the problem, not the goal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reimplementation using scanner
The scanner rewrite's `spine` helper was a lossy stand-in for the old indent rule: it never recorded the indent an operator break introduces, so a break nested inside an already-indented construct flushed back to column 0 (e.g. an `and` chain inside a piped `select(...)`). Replace it with a single rule — the indent is how many open groups are currently broken across lines — leaving the frame stack as the formatter's only state. Also break the line *before* `and`/`or` so the operator leads its line, as people write boolean chains. Pipe and comma still trail. Fixtures updated to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
This is no longer byte compatible with your outputs. Test cases show this, but want to call this out explicitly in case you decide to merge. |
- Break nested if/elif/else/end, not just top-level: drop the len(stack)==1
guard now that depth(stack) gives the right indent at any nesting.
- Don't misread keyword-named object keys ({and: 1}, {end: 2}) as
keywords/operators — track key vs value position on object groups and
gate the breaks on it, no lookahead.
- Rename frame/frames to group/stack; it's a stack of open groups, not
call frames.
- Verify formatting round-trips: re-parse breakLines output and error if
it doesn't canonicalize back, so a token bug can't ship silently.
Adds TestIfNested and TestObjectKeywordKeys.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Hi @noperator -
Useful lib, thanks! I was poking around with it since I needed something to reformat jq expressions. Worked well except for breaking if/else blocks.
I added that. In the process I was also thinking about this talk which I had listened to a while back. So, just for curiosity ended up thinking of the problem using the framing Rob had.
I have kept everything as-is in terms of licensing, flags, defaults, what they mean and such. You are welcome to accept/merge if you like the idea.
Heads up: this ended up as more of a reimplementation than a small patch — the behavior and outputs are the same, but I rewrote the internals around that framing rather than adding to the existing code. Totally understand if that's more than you want to take on.
If you want just the commit that fixes the if/else block, let me know, I'll open a different PR from that commit.