Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): add the includes field #4899

Open
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Jan 15, 2025

Summary

Part of #4611
This PR adds the includes field.
This PR is not a breaking change because I didn't remove include and ignore.

I leave the removal of include and ignore to another PR.
I also started a migration rule and decided to leave it for another PR.

I noted a minor "caveat": when we replace ignore with includes we have to add an include pattern to ensure that some files got processed.
For example, the following config:

{
  "files": {
    "ignore": ["src"]
  }
}

Have to be rewritten as:

{
  "files": {
    "includes": ["**", "!src"]
  }
}

I think it is fine.

Test Plan

I replaced all occurrences of include and ignore by includes in the tests.

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project A-Linter Area: linter A-Parser Area: parser A-Formatter Area: formatter A-Tooling Area: internal tools A-LSP Area: language server protocol L-JavaScript Language: JavaScript and super languages L-CSS Language: CSS L-JSON Language: JSON and super languages A-Diagnostic Area: diagnostocis A-Changelog Area: changelog L-HTML Language: HTML L-Grit Language: GritQL labels Jan 15, 2025
@Conaclos Conaclos changed the base branch from main to next January 15, 2025 20:51
@Conaclos Conaclos changed the title Conaclos/config includes next feat(config): add the includes field Jan 15, 2025
@Conaclos Conaclos force-pushed the conaclos/config-includes-next branch from 8848003 to 6b34864 Compare January 15, 2025 20:59
@github-actions github-actions bot removed A-Core Area: core A-Linter Area: linter A-Formatter Area: formatter A-Tooling Area: internal tools A-LSP Area: language server protocol L-CSS Language: CSS A-Diagnostic Area: diagnostocis A-Changelog Area: changelog L-HTML Language: HTML L-Grit Language: GritQL labels Jan 15, 2025
@Conaclos Conaclos force-pushed the conaclos/config-includes-next branch from 6b34864 to 53f9da6 Compare January 15, 2025 21:17
Copy link

codspeed-hq bot commented Jan 15, 2025

CodSpeed Performance Report

Merging #4899 will improve performances by 7.48%

Comparing conaclos/config-includes-next (abe70bf) with next (4f07cc8)

Summary

⚡ 1 improvements
✅ 94 untouched benchmarks

Benchmarks breakdown

Benchmark next conaclos/config-includes-next Change
react.production.min_3378072959512366797.js[cached] 2 ms 1.9 ms +7.48%

@Conaclos Conaclos force-pushed the conaclos/config-includes-next branch from 53f9da6 to d4f8b93 Compare January 16, 2025 20:14
@github-actions github-actions bot added A-Linter Area: linter A-LSP Area: language server protocol labels Jan 16, 2025
@Conaclos Conaclos marked this pull request as ready for review January 18, 2025 14:30
@Conaclos
Copy link
Member Author

Conaclos commented Jan 18, 2025

While I am testing the change, I noted some issues with our way of handling paths.

Given the following hierarchy:

.
└── src
    └── file.js

Biome will provide different paths to the glob matcher according to the command:

  • biome check . provides ., ./src, and ./src/file.js
  • biome check provides /absolute/path/, /absolute/path/src, and /absolute/path/src/file.js
  • biome check src provides src, and src/file.js
  • biome check ../../absolute/path provides ../../absolute/path, ../../absolute/path/src, and ../../absolute/path/src/file.js

This was not causing any issue with include/ignore because our glob library implicitly added ** in front of any glob.

We have to normalize the path provided on the CLI in order to solve the issue.
We should certainly normalize it against the path of the active configuration file biome.json.

I don't know how this affects our LSP.

Maybe related to the PR #4823 and the issue #4868?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Linter Area: linter A-LSP Area: language server protocol A-Parser Area: parser A-Project Area: project L-JavaScript Language: JavaScript and super languages L-JSON Language: JSON and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant