Skip to content

format: offer one-operand-per-line layout for and/or chains#1160

Merged
CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
MillhioreBT:binary_chain_operand
Jul 10, 2026
Merged

format: offer one-operand-per-line layout for and/or chains#1160
CppCXY merged 1 commit into
EmmyLuaLs:mainfrom
MillhioreBT:binary_chain_operand

Conversation

@MillhioreBT

Copy link
Copy Markdown
Contributor

Long and/or chains that don't fit on one line could break inside a nested operand's call args instead of at the chain's own operators:

if player:getStorageValue(Storage.A) >= 62 and player:getStorageValue(
        Storage.B
    ) >= 62 and player:removeItem(5022, 1) then

Adds an opt-in layout.prefer_binary_chain_operand_pechains of 3+ same-operator operands can breakone-per-line instead, operator leading:

if player:getStorageValue(Storage.A) >= 62
    and player:getStorageValue(Storage.B) >= 62
    and player:removeItem(5022, 1) then

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).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Issues Found

  1. Potential Logic Bug: Missing one_per_line field in SequenceLayout struct

    • The code references one_per_line in SequenceLayoutPolicy but there's no visible definition of this field in the diff. If SequenceLayout doesn't have this field, this will cause a compilation error.
    • Recommendation: Ensure the SequenceLayout struct has a one_per_line: Option<Vec<DocIR>> field defined.
  2. Unused Variable _space_before in build_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_before is intentional and doesn't cause formatting inconsistencies.
  3. Missing Edge Case: Single Operand Chain

    • build_binary_chain_one_per_line assumes operands has at least 1 element (accesses operands[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.
  4. 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_chain to ensure the chain has at least 3 operands before generating the one-per-line candidate.
  5. Test Coverage Gap: No Test for or Operator with Disabled Flag

    • There's a test for or with the flag enabled, but no regression test showing the legacy behavior for or chains when the flag is disabled.
    • Recommendation: Add a test similar to test_binary_chain_operand_per_line_disabled_keeps_legacy_behavior but for or chains.

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, or operator)
  • Consistent style: Uses leading-operator style (and/or at line start) matching existing project conventions

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@CppCXY CppCXY merged commit 7bf73b0 into EmmyLuaLs:main Jul 10, 2026
16 of 17 checks passed
@MillhioreBT MillhioreBT deleted the binary_chain_operand branch July 10, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants