Skip to content

Comments

refactor(oxfmt): Resolve options on each path, preparing for .editorconfig support#16991

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-17-refactor_oxfmt_prepare_for_.editorconfig_support
Dec 18, 2025
Merged

refactor(oxfmt): Resolve options on each path, preparing for .editorconfig support#16991
graphite-app[bot] merged 1 commit intomainfrom
12-17-refactor_oxfmt_prepare_for_.editorconfig_support

Conversation

@leaysgur
Copy link
Member

@leaysgur leaysgur commented Dec 17, 2025

Refactoring for #15850

Before this PR

  • The config file and its option values were resolved only once at the beginning
  • They were kept for OxcFormatter and also being cached via setupConfig() for ExternalFormatter
  • When calling SourceFormatter.format(), it always referenced those values

After this PR

  • The config file and its option values are resolved only once at the beginning
  • Those values are still kept as before
    • But, this is for a fast path when there are no per-path overrides in .editorconfig
  • When calling SourceFormatter.format()
    • If .editorconfig exists and has per-path overrides, they are re-resolved per path
    • Otherwise, it continues to reference the initially cached values

We only support the root .editorconfig, but since glob-based per-path overrides can be written there, we have to resolve options on each path. Some performance impact is unavoidable.

However, in scenarios where .editorconfig doesn't exist, or exists but has no per-path overrides, the cost is limited to just cloning the cached values per path.

@github-actions github-actions bot added A-cli Area - CLI A-formatter Area - Formatter labels Dec 17, 2025
Copy link
Member Author

leaysgur commented Dec 17, 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.

@github-actions github-actions bot added the C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior label Dec 17, 2025
@leaysgur leaysgur force-pushed the 12-17-refactor_oxfmt_prepare_for_.editorconfig_support branch 2 times, most recently from 7e40a73 to 5d2aa10 Compare December 17, 2025 08:30
@leaysgur leaysgur changed the title refactor(oxfmt): Prepare for .editorconfig support refactor(oxfmt): Resolve options on each path, preparing for .editorconfig support Dec 17, 2025
@leaysgur leaysgur marked this pull request as ready for review December 17, 2025 08:58
Copilot AI review requested due to automatic review settings December 17, 2025 08:58
@leaysgur leaysgur requested a review from Dunqing December 17, 2025 08:59
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 refactors the configuration resolution system in oxfmt to prepare for .editorconfig support. The key change is moving from resolving configuration once at the start to resolving it per-file, enabling future per-path overrides from .editorconfig.

Key Changes

  • Introduced ConfigResolver and ResolvedOptions types to handle configuration resolution per file path
  • Refactored external formatter callbacks to accept options on each call rather than during initialization
  • Updated all formatting entry points to resolve options per-file instead of using cached global options

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
apps/oxfmt/src/core/config.rs Introduced ConfigResolver and ResolvedOptions for per-file configuration resolution; moved from single load_config() to resolver pattern
apps/oxfmt/src/core/format.rs Updated SourceFormatter::format() to accept ResolvedOptions parameter; formatting methods now receive options per call
apps/oxfmt/src/core/external_formatter.rs Renamed callback from setup_config to init; formatter methods now accept options parameter on each call
apps/oxfmt/src/stdin/mod.rs Updated to use ConfigResolver and resolve options per stdin file entry
apps/oxfmt/src/main_napi.rs Updated NAPI API to use ConfigResolver and resolve options before formatting
apps/oxfmt/src/cli/format.rs Updated CLI runner to create ConfigResolver and pass to FormatService
apps/oxfmt/src/cli/service.rs Updated service to accept ConfigResolver and resolve options per file in parallel formatting
apps/oxfmt/src-js/prettier-proxy.ts Removed config setup from pool initialization; options now passed on each format call
apps/oxfmt/src-js/prettier-worker.ts Removed workerData config; workers now receive options with each task
apps/oxfmt/src-js/index.ts Updated to use initExternalFormatter instead of setupConfig
apps/oxfmt/src-js/cli.ts Updated callback reference to initExternalFormatter
apps/oxfmt/src-js/bindings.d.ts Updated TypeScript type definitions for new callback signatures

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

@leaysgur leaysgur force-pushed the 12-17-refactor_oxfmt_prepare_for_.editorconfig_support branch 2 times, most recently from 82edec0 to 95a3d1a Compare December 18, 2025 02:04
@leaysgur leaysgur force-pushed the 12-17-refactor_oxfmt_prepare_for_.editorconfig_support branch from 95a3d1a to bcff4b6 Compare December 18, 2025 05:59
Copy link
Member

@Dunqing Dunqing left a comment

Choose a reason for hiding this comment

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

Need to take a look at the Copilot review

@leaysgur
Copy link
Member Author

@Dunqing Oh, sorry, I missed that and resolved!

  • options mutation: It's safe and updated comment in the later PR
  • clone() perf: I locally tested Arc before, but it was not so effective, but I will check again later

@leaysgur leaysgur requested a review from Dunqing December 18, 2025 07:58
@graphite-app graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Dec 18, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 18, 2025

Merge activity

graphite-app bot pushed a commit that referenced this pull request Dec 18, 2025
…config` support (#16991)

Refactoring for #15850

### Before this PR
- The config file and its option values were resolved only once at the beginning
- They were kept for `OxcFormatter` and also being cached via `setupConfig()` for `ExternalFormatter`
- When calling `SourceFormatter.format()`, it always referenced those values

### After this PR
- The config file and its option values are resolved only once at the beginning
- Those values are still kept as before
  - But, this is for a fast path when there are no per-path overrides in `.editorconfig`
- When calling `SourceFormatter.format()`
  - If `.editorconfig` exists and has per-path overrides, they are re-resolved per path
  - Otherwise, it continues to reference the initially cached values

We only support the root `.editorconfig`, but since glob-based per-path overrides can be written there, we have to resolve options on each path. Some performance impact is unavoidable.

However, in scenarios where `.editorconfig` doesn't exist, or exists but has no per-path overrides, the cost is limited to just cloning the cached values per path.
@graphite-app graphite-app bot force-pushed the 12-17-refactor_oxfmt_prepare_for_.editorconfig_support branch from bcff4b6 to 7b9468b Compare December 18, 2025 10:40
…config` support (#16991)

Refactoring for #15850

### Before this PR
- The config file and its option values were resolved only once at the beginning
- They were kept for `OxcFormatter` and also being cached via `setupConfig()` for `ExternalFormatter`
- When calling `SourceFormatter.format()`, it always referenced those values

### After this PR
- The config file and its option values are resolved only once at the beginning
- Those values are still kept as before
  - But, this is for a fast path when there are no per-path overrides in `.editorconfig`
- When calling `SourceFormatter.format()`
  - If `.editorconfig` exists and has per-path overrides, they are re-resolved per path
  - Otherwise, it continues to reference the initially cached values

We only support the root `.editorconfig`, but since glob-based per-path overrides can be written there, we have to resolve options on each path. Some performance impact is unavoidable.

However, in scenarios where `.editorconfig` doesn't exist, or exists but has no per-path overrides, the cost is limited to just cloning the cached values per path.
@graphite-app graphite-app bot force-pushed the 12-17-refactor_oxfmt_prepare_for_.editorconfig_support branch from 7b9468b to 7368d66 Compare December 18, 2025 10:44
@graphite-app graphite-app bot merged commit 7368d66 into main Dec 18, 2025
18 checks passed
@graphite-app graphite-app bot deleted the 12-17-refactor_oxfmt_prepare_for_.editorconfig_support branch December 18, 2025 10:50
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-formatter Area - Formatter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants