format: offer one-operand-per-line layout for and/or chains#1160
Conversation
There was a problem hiding this comment.
Code Review Summary
Issues Found
-
Potential Logic Bug: Missing
one_per_linefield inSequenceLayoutstruct- The code references
one_per_lineinSequenceLayoutPolicybut there's no visible definition of this field in the diff. IfSequenceLayoutdoesn't have this field, this will cause a compilation error. - Recommendation: Ensure the
SequenceLayoutstruct has aone_per_line: Option<Vec<DocIR>>field defined.
- The code references
-
Unused Variable
_space_beforeinbuild_binary_chain_one_per_line- Line:
let (_space_before, segment) = build_binary_chain_segment(...) - The underscore prefix suggests intentional ignoring, but this discards spacing information that might be relevant for formatting consistency.
- Recommendation: Verify that ignoring
_space_beforeis intentional and doesn't cause formatting inconsistencies.
- Line:
-
Missing Edge Case: Single Operand Chain
build_binary_chain_one_per_lineassumesoperandshas at least 1 element (accessesoperands[0]), but the function is only called for chains with 3+ operands. However, if the chain detection logic changes, this could panic.- Recommendation: Add a guard clause or assertion at the start of the function.
-
Documentation Inconsistency in Chinese Docs
- The Chinese documentation mentions "多元二元表达式链(3 个及以上操作数)" but the English docs say "3 or more operands". The implementation doesn't explicitly enforce this minimum - it's only implied by the calling context.
- Recommendation: Add an explicit check in
try_format_flat_binary_chainto ensure the chain has at least 3 operands before generating the one-per-line candidate.
-
Test Coverage Gap: No Test for
orOperator with Disabled Flag- There's a test for
orwith the flag enabled, but no regression test showing the legacy behavior fororchains when the flag is disabled. - Recommendation: Add a test similar to
test_binary_chain_operand_per_line_disabled_keeps_legacy_behaviorbut fororchains.
- There's a test for
Positive Aspects
- Clean separation of concerns: The new layout candidate is isolated in its own function
build_binary_chain_one_per_line - Backward compatible: Default value is
false, preserving existing behavior - Well-documented: Both Chinese and English documentation updated with clear behavior notes
- Comprehensive tests: Multiple test cases covering different scenarios (short chains, long chains, disabled flag,
oroperator) - Consistent style: Uses leading-operator style (
and/orat line start) matching existing project conventions
There was a problem hiding this comment.
Code Review
This pull request introduces a new formatting configuration option, prefer_binary_chain_operand_per_line, which offers a one-operand-per-line layout candidate for binary chains sharing the same operator (such as a sequence of and/or conditions). This option helps prevent less-than-ideal breaking inside nested operands when a long chain does not fit on a single line. The PR includes implementation details in the formatter, comprehensive unit tests covering various layout scenarios, and updated documentation in both English and Chinese. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Long
and/orchains that don't fit on one line could break inside a nested operand's call args instead of at the chain's own operators:Adds an opt-in layout.prefer_binary_chain_operand_pechains of 3+ same-operator operands can breakone-per-line instead, operator leading:
Just adds a candidate to the existing layout scorer — flat/fill/packed still win when they fit in fewer lines, so short chains are unaffected. Default false, no behavior change unless opted in.
6 tests + docs (options_EN.md / options_CN.md).