feat(config): add the includes field

[Conaclos]

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.

[Conaclos]

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?

[ematipico]

@Conaclos

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

I was actually working on this PR before you mentioned that you were working on it, and I realised this, and I had already fixed the bug.

When converting the Glob from biome_configuration::Configuration to Settings, we must strip the working directory from the input path (the path that we are handling). Hence, I had created a small newtype that could hold the working directory (which is the biome.json directory, as you mentioned), and a new method that strips the working directory from input path, and then use the result against the Glob type.

We must apply this change in this PR, and once you'll do so we will have two tests that will break. You will realise that those regressions make sense because we don't apply ** before the glob, so we will need to update the test cases.

[Conaclos]

@ematipico Thanks for the hint. Do you want to port your solution to this PR?

We must apply this change in this PR, and once you'll do so we will have two tests that will break. You will realise that those regressions make sense because we don't apply ** before the glob, so we will need to update the test cases.

I already updated the failing tests.

[ematipico]

I will, thank you!

[ematipico]

@Conaclos

I pushed my changes in bc3bdb7 (#4899)

Highlight of the changes:

• I had to change the signature of CandidatePath::new to exactly match the one of globset::Candidate because it the compiler was complaining about &UtfPath8 not knowing its size at compile time.
• I created a new type called IncludesFiles which contains two high-order functions that call the underneath CandidatePath functions, that's because we need to strip the working directory from the paths, since we don't prepend ** to our globs
• I also left a note about the default values coming from the CLI. I believe the issue is caused by pure(Default::default()). I tried to use different approaches, but they didn't workout. Maybe it's a bug in bpaf but I didn't spend time creating a minimal reproduction

[Conaclos]

@ematipico

I pushed my changes in bc3bdb7 (#4899)

Is it normal that you didn't replace all occurrences of Box<[biome_glob::Glob]> with IncludesFiles? Notably in FormatSettings, LinterSettings and AssistSettings.

I created a new type called IncludesFiles which contains two high-order functions that call the underneath CandidatePath functions, that's because we need to strip the working directory from the paths, since we don't prepend ** to our globs

Do you think we could eventually move the normalization in a higher level?
For now, it is fine. However, I wonder if we could avoid repeated normalization of a same path. Moreover, I would like eventually to pass CandidatePath instead of Utf8Path to avoid repeated conversion of Utf8Path to CandidatePath. Of course these changes should be left for a future PR (they are improvements not critical to the feature).

[ematipico]

Is it normal that you didn't replace all occurrences of Box<[biome_glob::Glob]> with IncludesFiles?

I definitely missed them! Ooops

Do you think we could eventually move the normalization in a higher level?

I think so, but that's not part of this PR but the traversal/workspace.

Maybe we could strip the workspace directory from the file path inside the open_file method

[Conaclos]

I definitely missed them! Ooops

Fixed :)

[pacak]

I tried to use different approaches, but they didn't workout. Maybe it's a bug in bpaf but I didn't spend time creating a minimal reproduction

Released 0.9.16 :)

[chansuke]

q: are we no longer using the constant here to improve maintainability (switching from an ignore-based configuration to an includes-based one with a negation)?

[Conaclos]

I am not sure to understand what you mean.

This function is a test, I chose to use an inline configuration because this makes it easier to understand and edit the test.
This makes the test self-contained.
I don't know why we use constants in the first place: maybe to factorize some tests?
Most of them are only used by a single test.

[Conaclos]

I am going to merge the PR. Please let me know if I missed something. I will fix it in a future PR if needed.

[Conaclos]

I updated bpaf, removed the workaround, added some documentation and did some refactoring. The PR is ready to merge.