Skip to content

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented Oct 22, 2025

As a result, it caused a parsing error, and nothing was recorded. 🙈

  • tests/fixtures/js/comments/yield.js
    • Removed, the same test already exists in ts/comments/yield.ts
  • tests/fixtures/js/comments/assignments.js
    • Removed TS syntax
  • tests/fixtures/ts/comments/assignments.ts
    • The same as original js/comments/assignment.js

@leaysgur leaysgur requested a review from Dunqing as a code owner October 22, 2025 06:04
@github-actions github-actions bot added A-formatter Area - Formatter C-test Category - Testing. Code is missing test cases, or a PR is adding them labels Oct 22, 2025
Copy link
Member Author

leaysgur commented Oct 22, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@leaysgur leaysgur changed the title test(formatter): Fix tests using TS syntax with .js test(formatter): Fix tests using TS syntax with .js Oct 22, 2025
@leaysgur
Copy link
Member Author

For yield.(j|t)s case, I may find mismatches vs Prettier & Biome.

// ORIGINAL
function *t1() {
    yield (
        // comment
        a
    );
}

// Oxc
function* t1() {
  yield (
    // comment
    a
  );
}

// Prettier
function* t1() {
	yield // comment
	a;
}

// Biome
function* t1() {
	yield// comment
	a;
}

I'm not sure it might just be a known issue, so I'll leave this to you. 🙏🏻

@Dunqing
Copy link
Member

Dunqing commented Oct 22, 2025

For the yield file, we should move it to the ts folder and rename it to yield.ts. This is what I want to test

@Dunqing
Copy link
Member

Dunqing commented Oct 22, 2025

For yield.(j|t)s case, I may find mismatches vs Prettier & Biome.

// ORIGINAL
function *t1() {
    yield (
        // comment
        a
    );
}

// Oxc
function* t1() {
  yield (
    // comment
    a
  );
}

// Prettier
function* t1() {
	yield // comment
	a;
}

// Biome
function* t1() {
	yield// comment
	a;
}

I'm not sure it might just be a known issue, so I'll leave this to you. 🙏🏻

It is expected, and probably a bug in the Prettier.

@leaysgur
Copy link
Member Author

For the yield file, we should move it to the ts folder and rename it to yield.ts. This is what I want to test

As I noted top comment, there is already yield.ts file. 👌🏻

So, is it OK to remove yield.js?

@Dunqing
Copy link
Member

Dunqing commented Oct 22, 2025

As I noted top comment, there is already yield.ts file. 👌🏻

So, is it OK to remove yield.js?

Oh, sorry, I overlooked. Now it is okay to remove.

@leaysgur leaysgur force-pushed the 10-22-feat_oxfmt_more_friendly_json_schema branch from 229b2be to 5442349 Compare October 22, 2025 08:17
@leaysgur leaysgur force-pushed the 10-22-test_formatter_fix_tests_using_ts_syntax_with_.js branch from a0daa6a to 839688e Compare October 22, 2025 08:17
@leaysgur
Copy link
Member Author

Removed. Now let me merge the stack. 🕺🏻

@leaysgur leaysgur added the 0-merge Merge with Graphite Merge Queue label Oct 22, 2025
Copy link
Member Author

leaysgur commented Oct 22, 2025

Merge activity

@graphite-app graphite-app bot changed the base branch from 10-22-feat_oxfmt_more_friendly_json_schema to graphite-base/14880 October 22, 2025 08:25
@graphite-app graphite-app bot force-pushed the 10-22-test_formatter_fix_tests_using_ts_syntax_with_.js branch from 839688e to fe1a101 Compare October 22, 2025 08:31
@graphite-app graphite-app bot force-pushed the graphite-base/14880 branch from 5442349 to 381e08c Compare October 22, 2025 08:31
@graphite-app graphite-app bot changed the base branch from graphite-base/14880 to main October 22, 2025 08:31
@graphite-app graphite-app bot force-pushed the 10-22-test_formatter_fix_tests_using_ts_syntax_with_.js branch from fe1a101 to faec008 Compare October 22, 2025 08:32
As a result, it caused a parsing error, and nothing was recorded. 🙈

- tests/fixtures/js/comments/yield.js
  - Removed, the same test already exists in ts/comments/yield.ts
- tests/fixtures/js/comments/assignments.js
  - Removed TS syntax
- tests/fixtures/ts/comments/assignments.ts
  - The same as original js/comments/assignment.js
@graphite-app graphite-app bot force-pushed the 10-22-test_formatter_fix_tests_using_ts_syntax_with_.js branch from faec008 to 868ff99 Compare October 22, 2025 08:38
Copilot AI review requested due to automatic review settings October 22, 2025 08:38
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@graphite-app graphite-app bot merged commit 868ff99 into main Oct 22, 2025
18 checks passed
@graphite-app graphite-app bot deleted the 10-22-test_formatter_fix_tests_using_ts_syntax_with_.js branch October 22, 2025 08:44
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Oct 22, 2025
@Boshen Boshen mentioned this pull request Oct 22, 2025
graphite-app bot pushed a commit that referenced this pull request Oct 22, 2025
## [0.8.0] - 2025-10-22

### 🚀 Features

- 381e08c oxfmt: More friendly JSON schema (#14879) (leaysgur)
- 006708d oxfmt: Support `ignorePatterns` in oxfmtrc (#14875) (leaysgur)

### 🐛 Bug Fixes

- 64b8226 formatter: Corrct printing leading own line comments before method body (#14886) (Dunqing)
- 6ce1162 formatter: Remove a redundant space for TSMappedType (#14885) (Dunqing)
- 5b962a7 formatter: Remove redundant leading space when only the rest part of the array exists (#14884) (Dunqing)
- 8301d8f formatter: No need to wrap parenthesis for ObjectExpression when it is an expression of a template literal (#14883) (Dunqing)
- 9397472 formatter: Should not wrap parenthesis for ComputedMemberExpression when it is not an option or it doesn't contain a call expression (#14882) (Dunqing)
- 3e62277 formatter: Should not add a soft line for the arrow function inside ExpressionContainer with a trailing comment (#14878) (Dunqing)
- 990916a formatter: Correct handling of leading own line before arrow function body (#14877) (Dunqing)
- 4a499b5 formatter: Correct printing trailing comments for if statement with non-block consequent (#14857) (Dunqing)

### 🧪 Testing

- 868ff99 formatter: Fix tests using TS syntax with `.js` (#14880) (leaysgur)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-formatter Area - Formatter C-test Category - Testing. Code is missing test cases, or a PR is adding them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants