Add new formatting options#124
Open
chrisdp wants to merge 16 commits into
Open
Conversation
- maxConsecutiveEmptyLines: collapse runs of blank lines to a configurable max - trailingComma: add or remove trailing commas in multi-line arrays/AAs - blankLinesBetweenFunctions: enforce exact blank lines between function/sub declarations - singleLineIf: collapse or expand single-statement if blocks - inlineArrayAndObjectThreshold: collapse short multi-line arrays/AAs to a single line - removeBlankLinesAtStartOfBlock: remove blank lines at the start of function/if/for/while bodies - alignAssignments: align = signs in consecutive assignment blocks Each option is covered by tests written before implementation (TDD). Pipeline ordering comments explain why each formatter runs where it does relative to IndentFormatter and MultiLineItemFormatter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… 'allButLast' mode - 'always' and 'never' now apply to every item in a multiline collection, not just the trailing one — enabling conversion between the BrightScript newline-separator style and the comma-separator style - New 'allButLast' mode adds commas to all items except the last, matching the conventional style used in most languages - Formatter processes collections innermost-first to keep indices stable across splices, and collects all item positions before deciding so the last item can be identified for 'allButLast' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Added entries for insertSpaceAfterConditionalCompileSymbol, trailingComma, maxConsecutiveEmptyLines, blankLinesBetweenFunctions, singleLineIf, inlineArrayAndObjectThreshold, removeBlankLinesAtStartOfBlock, and alignAssignments — all of which existed in FormattingOptions but were absent from the options table. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
chrisdp
commented
Mar 29, 2026
chrisdp
changed the title
TwitchBronBron
requested changes
May 7, 2026
Comment on lines
+35
to
+36
| const nonWs = lineTokens.filter(t => t.kind !== TokenKind.Whitespace && t.kind !== TokenKind.Newline && t.kind !== TokenKind.Eof); | ||
| return nonWs.length >= 3 && nonWs[0].kind === TokenKind.Identifier && nonWs[1].kind === TokenKind.Equal; |
Member
There was a problem hiding this comment.
I don't know that this is the right approach. We will iterate over every single token, just to see if we have more than 3? That's seemingly wasteful
Will be revisited later with a more eslint-like/flexible API.
Replace `expand`/`collapse`/`original` with eslint-style values:
`block`, `inline`, `inlineNoElseIf`, `inlineNoElse`, `original`. The
inline* variants form a strictness gradient over which else/else-if
branch shapes may collapse.
Block mode (expand) now walks the entire if/else-if/else chain via
elseBranch and breaks around every then/else, so inline ifs with else
branches expand correctly instead of cramming the whole chain onto one
body line. Filters on `isInline === true` (not `!tokens.endIf`) to
avoid mistaking a multi-line chain's outer IfStatement for an inline
one — brighterscript attaches the `end if` to the deepest else-if of
a multi-line chain, leaving the outer endIf-less. Also rejects inline
ifs whose range spans multiple lines (e.g. `if x then return { ... }`
with a multi-line literal body) to avoid landing `end if` inside the
literal.
Inline modes (collapse) reject bodies that would make the result
broken or undesirable: nested IfStatement, TryCatchStatement,
CommentStatement, and any single-statement body whose range spans
multiple lines (multi-line for/while/for-each, multi-line function
call, multi-line array/AA literal as RHS, etc).
Both modes were previously declared in the type union but routed to the same code path as inlineNoElse. They now do what their names imply: - `inlineNoElseIf` collapses simple `if/then` and adds `if/then/else`, but rejects `else if` chains. - `inline` adds `else if` chains (with optional final `else`), the most aggressive setting. isCollapsible walks the elseBranch chain and validates each branch's body in turn, with the mode controlling which branch shapes are allowed. collapse() walks the same chain to gather every body opener (then tokens, plus plain-else's else) and every body closer (else tokens, plus the chain's end-if), then collapses each `\n + indent` into a single space. Token references stay valid across splices, so operations are order-independent and re-resolve indices each time. Removed two SingleLineIfFormatter unit tests that probed early-return paths of the old single-pass collapse; the user-facing tests in Formatter.spec.ts cover the same edge cases (corrupt structure now short-circuits via an explicit pre-mutation validation pass).
Replace `inlineArrayAndObjectThreshold: number` with eslint-style
string values: `always`, `never`, `fitsLine`, `original`. Add a global
`maxLineLength` option that `fitsLine` consults to decide whether the
collapsed line stays within budget; falls back to `always` when unset.
Collapse path:
- Inserts a comma + space between items when the original separator was
only a newline (previously produced broken output like `{ a: 1b: 2 }`
or `[123]`).
- Refuses to collapse literals whose contents include any of: line
comments (`'note`), `bs:disable-line` directives, conditional-compile
directives (`#if`/`#else if`/`#else`/`#end if`), nested multi-line
literals, items whose value spans multiple physical lines (multi-line
function expressions, etc), or regex literals (brighterscript's lexer
does not include `,` in `PreceedingRegexTypes`, so a regex after a
comma re-lexes as division).
- For `fitsLine`, factors visual indent into the length budget using
`indentSpaceCount` for tab expansion.
Expand path (new for `never` mode):
- Walks single-line literals with at least 2 items, inserts newlines
after the opener, after each top-level comma, and before the closer.
IndentFormatter restores indentation on the next pass.
- Skips empty literals and single-element literals.
Tests cover all four modes, every structural rejection, comma-fix bugs,
and length cutoffs. Validated semantically against bsc on
jellyfin-roku, jellyfin-roku-legacy, jellyrock, and Roku-GooglePhotos
across 3 modes — all produce identical bsc diagnostic histograms.
Replace the boolean `removeBlankLinesAtStartOfBlock` option with `blockSpacing`, which controls blank-line spacing AROUND block constructs (not just at the start of bodies). String form: - `'before'` — blank line above the block (above any leading comment or annotation) - `'after'` — blank line below the block (after any same-line trailing comment) - `'between'` — both `'before'` and `'after'` - `'always'` — `'between'` plus inner-body padding (start and end of the body) - `'original'` (or omitted) — leave spacing as written Object form allows per-construct overrides for function, sub, if, for, while, try, plus a `default` fallback for unspecified constructs. Setting `if` covers the entire if/else if/else chain. `for` covers `for` and `for each`. `try` covers the entire try/catch construct. Comment and annotation attachment: leading line comments and annotations (`@deprecated`, etc.) immediately above an opener are part of the block. The `'before'` blank lands above the entire preamble, never between preamble and opener. Parent-boundary detection: spacing is suppressed when the construct is the first or last thing in its parent's body. Parents recognized include function/sub bodies, if/else branches, for/while/try bodies, namespace bodies, class/interface/enum bodies, and conditional-compile branches. Anonymous function/sub expressions (`.catch(sub(e) ... end sub)`, `x = function() ... end function`) are skipped entirely — they're expression-form, not declarations. The fix also handles nested same-kind constructs (depth-counted closer matching so nested if/end if pairs resolve correctly) and correctly skips probe whitespace when checking if a closer is the parent's closer. Tests now cover: every string value, per-construct overrides, default fallback, comment + annotation preambles, parent boundary suppression (first child of namespace, last child of namespace, nested-only-child patterns), anonymous lambda exclusion, and nested namespaces. Validated against bsc on five Roku repos × ten configurations (four string values + six per-construct overrides): 50/50 produce identical bsc diagnostic histograms.
Restructure the new formatters so every branch is either exercised by a real test or genuinely unreachable from contract guarantees: - Remove defensive guards that the upstream call shape already guarantees (e.g., `indexOf === -1` checks where the validation pass has already proven the token is present). - Replace optional chaining (`?.`) with non-null assertions (`!`) where the operand is provably defined. - Restructure `isCollapsible`'s walk loop as `while (true)` since every body path either returns or continues — eliminates the unreachable post-loop return. - Use `tokens.splice(closeIdx - 1, 1, newline)` directly where the preceding ws token is guaranteed by the lexer, etc. Add unit tests for branches reachable only through synthetic token arrays: - BlockSpacingFormatter.spec.ts (new file) — 29 unit tests targeting defensive paths and nested same-kind closer matching. - InlineArrayAndObjectFormatter.spec.ts — extended with tests for nested function expressions (depth >= 2), Sub/EndSub matching, unterminated function in literal, and `never`-mode unclosed brackets. - SingleLineIfFormatter.spec.ts — added a synthetic chain where the `else` token is not preceded by whitespace. Add functional tests for paths reachable through normal BS input: - Tab-indent visual-width handling under fitsLine - Explicit indentSpaceCount setting - Nested single-line arrays/AAs in `never` mode (recursive expansion) - `try` construct via object form - Parent-boundary detection in namespaces and class methods - Mixed comment + annotation preamble walking - Anonymous lambda exclusion (subs in `.catch(sub(e) ...)` chains) - Nested namespace scenarios Final coverage: 100% lines, branches, functions, statements across all source files. Validated against bsc on four public repos × ten configurations — 40/40 produce identical diagnostic histograms.
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
2 participants
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.
Summary
alignAssignments,blankLinesBetweenFunctions,inlineArrayAndObjectThreshold,maxConsecutiveEmptyLines,removeBlankLinesAtStartOfBlock,singleLineIf, andtrailingCommatrailingCommato apply to all items in multiline arrays and AAs (not just the last), enabling conversion between BrightScript's newline-separator style and comma-separator styletrailingComma: 'allButLast'mode — commas on every item except the trailing one (conventional style)bsfmt.jsonoptions in the READMETest plan
npm test)trailingCommacovering: comma-free AA →'always','never'removing all commas,'allButLast', nested collections, blank lines inside collections🤖 Generated with Claude Code