Skip to content

fix(formatter): method signature missed a semicolon when it is on the same line with a property#17243

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-22-fix_formatter_method_signature_missed_a_semicolon_when_it_is_on_the_same_line_with_a_property
Dec 22, 2025
Merged

fix(formatter): method signature missed a semicolon when it is on the same line with a property#17243
graphite-app[bot] merged 1 commit intomainfrom
12-22-fix_formatter_method_signature_missed_a_semicolon_when_it_is_on_the_same_line_with_a_property

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Dec 22, 2025

close: #17151

@github-actions github-actions bot added A-formatter Area - Formatter C-bug Category - Bug labels Dec 22, 2025
Copy link
Member Author

Dunqing commented Dec 22, 2025

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 22, 2025

CodSpeed Performance Report

Merging #17243 will not alter performance

Comparing 12-22-fix_formatter_method_signature_missed_a_semicolon_when_it_is_on_the_same_line_with_a_property (54d12e5) with main (e08fdf1)1

Summary

✅ 38 untouched
⏩ 7 skipped2

Footnotes

  1. No successful run was found on main (879601e) during the generation of this report, so e08fdf1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

  2. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a formatter issue where method signatures were missing semicolons when appearing on the same line as a property in TypeScript type definitions. The fix refactors the semicolon insertion logic to be more maintainable while preserving the original behavior.

  • Refactored the Semicolons::AsNeeded logic from an early-return pattern to a more concise matches! expression
  • Added test cases demonstrating method signatures and properties on the same line
  • Updated test snapshots to reflect the correct formatting behavior

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
crates/oxc_formatter/src/write/mod.rs Refactored semicolon insertion logic in FormatTSSignature::fmt to use matches! macro instead of early return pattern, improving code maintainability
crates/oxc_formatter/tests/fixtures/ts/semi/method-signatures.ts Added test case with union type containing method signatures and properties on the same line
crates/oxc_formatter/tests/fixtures/ts/semi/method-signatures.ts.snap Updated snapshot to include expected formatting output for the new test case across different configurations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

How come there were no existing test failures? An idempotent test should've caught this somewhere.

Copy link
Member

@leaysgur leaysgur left a comment

Choose a reason for hiding this comment

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

https://github.com/prettier/prettier/blob/0b16f5d33936b83db5cb87e9b7b8755040f4af06/tests/format/typescript/interface/no-semi/14040.ts#L77-L80

According to my quick research, this is the most relevant Prettier test case.

However, since the method signature in question is located at the end, it was not detected.
(In other words, it wasn't even tested in Prettier.)

@Dunqing
Copy link
Member Author

Dunqing commented Dec 22, 2025

How come there were no existing test failures? An idempotent test should've caught this somewhere.

#17151 (comment)

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 22, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the 12-22-fix_formatter_method_signature_missed_a_semicolon_when_it_is_on_the_same_line_with_a_property branch from 54d12e5 to 6d03edd Compare December 22, 2025 09:04
@graphite-app graphite-app bot merged commit 6d03edd into main Dec 22, 2025
20 checks passed
@graphite-app graphite-app bot deleted the 12-22-fix_formatter_method_signature_missed_a_semicolon_when_it_is_on_the_same_line_with_a_property branch December 22, 2025 09:10
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-formatter Area - Formatter C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

formatter: Broken result (and diff with prettier) on type object

4 participants