fix: support ignorePatterns in both rule options or config file#8
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
📝 WalkthroughWalkthroughThis PR implements ignorePatterns support for the oxfmt ESLint plugin, enabling it to respect ignore patterns defined in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ignorePatterns in both rule config or config fileignorePatterns in both rule config or options file
commit: |
ignorePatterns in both rule config or options fileignorePatterns in both rule options or config file
There was a problem hiding this comment.
Pull request overview
Adds support for honoring ignorePatterns when the formatter is run through the oxfmt/oxfmt ESLint rule, aligning ESLint behavior with oxfmt --check and addressing issue #7.
Changes:
- Implement
ignorePatternshandling in the oxfmt worker (supports both rule options and config-loaded patterns). - Add/extend tests and fixtures to verify ignore behavior with
useConfig: trueand rule-level precedence. - Update tooling/dependencies (tsdown config option, package bumps) and CI autofix workflow.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
workers/oxfmt.mjs |
Adds shouldIgnoreFile() and early-return behavior to skip formatting for ignored files. |
tsdown.config.ts |
Updates tsdown dependency bundling config key (onlyBundle). |
tests/rules/oxfmt.test.ts |
Adds rule-tester cases covering ignored vs non-ignored files and multiple ignore patterns. |
tests/rules/__snapshots__/oxfmt.test.ts.snap |
Updates snapshots for the new invalid test case. |
tests/eslint-plugin.test.ts |
Adds integration tests verifying .oxfmtrc ignorePatterns and rule-level precedence. |
tests/__snapshots__/eslint-plugin.test.ts.snap |
Adds snapshots for the new integration tests. |
tests/fixtures/use-config/src/unformatted.js |
Fixture for “should be formatted” when useConfig loads ignorePatterns. |
tests/fixtures/use-config/ignored/unformatted.js |
Fixture for “should be ignored” via .oxfmtrc ignorePatterns. |
tests/fixtures/use-config/.oxfmtrc.jsonc |
Adds ignorePatterns config used by the new integration tests. |
tests/fixtures/base/ts.ts |
Adds a base fixture used by the updated linting integration test. |
package.json |
Bumps dependencies (notably load-oxfmt-config, oxfmt, tsdown) and pnpm version. |
pnpm-lock.yaml |
Lockfile updates to reflect dependency/tooling bumps. |
eslint.config.mjs |
Simplifies repo ESLint config and enables oxfmt in @ntnyq/eslint-config. |
.vscode/settings.json |
Updates the tsgo setting key. |
.oxfmtrc.jsonc |
Adjusts ignorePatterns and JSONC formatting (commas/trailing commas). |
.github/workflows/autofix.yml |
Runs pnpm run format in autofix workflow and bumps autofix-ci/action. |
You can also share your feedback on Copilot code review. Take the survey.
| shouldIgnoreFile( | ||
| filename, | ||
| cwd, | ||
| ignorePatterns || baseOptions.ignorePatterns, |
There was a problem hiding this comment.
ignorePatterns || baseOptions.ignorePatterns treats an explicitly provided empty array (ignorePatterns: []) as falsy and will fall back to the config-file ignorePatterns. This makes it impossible to intentionally disable ignore patterns from config via rule options. Use nullish coalescing (ignorePatterns ?? baseOptions.ignorePatterns) (or an explicit check for undefined) so an empty array is respected as an override.
| ignorePatterns || baseOptions.ignorePatterns, | |
| ignorePatterns ?? baseOptions.ignorePatterns, |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/eslint-plugin.test.ts (1)
61-63: Consider adding explicit assertions for expected file count.Both tests rely on glob finding the expected fixture files but don't verify the count. Adding a simple assertion like
expect(files.length).toBe(2)would catch issues if fixture files are accidentally removed or renamed, making test failures more obvious.💡 Optional: Add file count assertion
const results = await eslint.lintFiles(files) + expect(files.length).toBe(2) results.forEach((result, idx) => { expect(result.messages).toMatchSnapshot(files[idx]) })Also applies to: 96-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/eslint-plugin.test.ts` around lines 61 - 63, Add an explicit assertion that the number of fixture files found matches the expected count before iterating results; e.g., in tests/eslint-plugin.test.ts where results.forEach((result, idx) => expect(result.messages).toMatchSnapshot(files[idx])), add a check like expect(files.length).toBe(<expectedCount>) (and the analogous assertion in the second test block that also iterates results) so the tests fail clearly if fixture files are removed or renamed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/eslint-plugin.test.ts`:
- Around line 61-63: Add an explicit assertion that the number of fixture files
found matches the expected count before iterating results; e.g., in
tests/eslint-plugin.test.ts where results.forEach((result, idx) =>
expect(result.messages).toMatchSnapshot(files[idx])), add a check like
expect(files.length).toBe(<expectedCount>) (and the analogous assertion in the
second test block that also iterates results) so the tests fail clearly if
fixture files are removed or renamed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2dcddd7a-c75d-4839-9f16-10cdc12f50a0
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/__snapshots__/eslint-plugin.test.ts.snapis excluded by!**/*.snaptests/rules/__snapshots__/oxfmt.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
.github/workflows/autofix.yml.oxfmtrc.jsonc.vscode/settings.jsoneslint.config.mjspackage.jsontests/eslint-plugin.test.tstests/fixtures/base/ts.tstests/fixtures/use-config/.oxfmtrc.jsonctests/fixtures/use-config/ignored/unformatted.jstests/fixtures/use-config/src/unformatted.jstests/rules/oxfmt.test.tstsdown.config.tsworkers/oxfmt.mjs
fixes: #7
Summary by CodeRabbit
New Features
Chores