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: support for nested ignore files #5195

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Feb 24, 2025

Summary

Closes #2312

This PR enables Biome to read and understand nested .gitignore files, and eventually ignore them in CLI and LSP.

In order to achieve that, I applied the following refactors:

  • created a new ExtensionHandler that understands ignore files.
  • updated DocumentFileSource, and added a new function called can_read. This function is used in the following places
    • Inside open_file_internal. We bail if Biome can't read a certain file
    • Inside can_handle of the traverse.rs file. Now Biome won't throw an error if it can read a file
  • Changed the type of Document::syntax, inside the workspace server. This type holds the content a file, and its parsed tree. Now, the parsed tree is a Option, because a file can be read from disk, but can't be parsed. This the case for ignore files.
  • The VCS configuration is now mapped into a new type called VcsSettings. These settings are used to query if a file is ignored.
  • The WorkspaceServer now has a new method called update_project_ignore_files, which is in charge of updating the project Settings::vcs_settings with the ignore files that were discovered after the project was scanned. This method does what the former method did: read the file from disk, and create a Gitignore type. As per the new feature, we now have a root Gitignore, the ignore file found in the project's root. Then we have a list of Gitgnore, which represents the nested ignore files; these are different because they must be matched against the folder they are contained to, so they are treated differently.

Test Plan

We have very similar tests that were repeated across CLI commands, I removed them and placed them inside the cases/ folder, using the TemporaryFs, which should help with the testing.

I also tested it manually using our own repository. I did remove dist/ from the ignore file, and added a new .gitignore inside the tailwind package. The CLI correctly ignores the file inside the dist/ folder (created upon compilation). I also checked that VSCode, upon file opening, doesn't provide diagnostics and doesn't format the files. Fortunately the logs are helpful and I could see that the file was ignored.

@github-actions github-actions bot added A-CLI Area: CLI A-Core Area: core A-Project Area: project 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 labels Feb 24, 2025
@ematipico ematipico marked this pull request as ready for review February 24, 2025 16:39
@ematipico ematipico requested review from a team February 24, 2025 16:40
@siketyan
Copy link
Member

Some dbg! and println! calls are not removed? 👀

@ematipico ematipico force-pushed the chore/multiple-ignores branch from 6359dd1 to 25ae4d9 Compare February 24, 2025 16:53
@ematipico
Copy link
Member Author

Some dbg! and println! calls are not removed? 👀

Addressed, I had the commit not pushed. Thank you!

@ematipico ematipico force-pushed the chore/multiple-ignores branch from 25ae4d9 to 770b708 Compare February 24, 2025 17:02
Copy link
Contributor

github-actions bot commented Feb 24, 2025

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 50387 50387 0
Passed 49072 49072 0
Failed 1315 1315 0
Panics 0 0 0
Coverage 97.39% 97.39% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6638 6638 0
Passed 2229 2229 0
Failed 4409 4409 0
Panics 0 0 0
Coverage 33.58% 33.58% 0.00%

ts/babel

Test result main count This PR count Difference
Total 798 798 0
Passed 704 704 0
Failed 94 94 0
Panics 0 0 0
Coverage 88.22% 88.22% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 18655 18655 0
Passed 14306 14306 0
Failed 4349 4349 0
Panics 0 0 0
Coverage 76.69% 76.69% 0.00%

Copy link

codspeed-hq bot commented Feb 24, 2025

CodSpeed Performance Report

Merging #5195 will not alter performance

Comparing chore/multiple-ignores (08b771a) with main (43601b2)

Summary

✅ 97 untouched benchmarks

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Nice!

@ematipico ematipico requested review from siketyan, arendjr and a team February 25, 2025 15:51
Copy link
Member

@siketyan siketyan left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for the great improvement 🚀

Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

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

Just two last comments 👍

@ematipico ematipico force-pushed the chore/multiple-ignores branch from 443d233 to 08b771a Compare February 26, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CLI Area: CLI A-Core Area: core A-Formatter Area: formatter A-LSP Area: language server protocol A-Parser Area: parser A-Project Area: project A-Tooling Area: internal tools L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vcs.useIgnoreFile ignores nested ignore files
3 participants