-
Notifications
You must be signed in to change notification settings - Fork 18
feat(core): support nested .gitignore files in filtering #2156
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 571dc60 The changes in this PR will be included in the next version bump. This PR includes changesets to release 25 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] This is a lot of added logic. Back in #1856 (comment) I'd found a couple of other packages: gitignore-to-glob, globify-gitignore. Regardless of what package we use, I think it would be nice to have to manage this logic ourselves. Did you try out those packages and/or others? Is there some reason they aren't workable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd found a couple of other packages: gitignore-to-glob, globify-gitignore.
Both of them are no longer maintained. The logic also isn’t particularly complex — for example, gitignore-to-glob is only around 80 lines of code.
Regardless of what package we use, I think it would be nice to have to manage this logic ourselves.
100 percent agree.
Did you try out those packages and/or others? Is there some reason they aren't
I didn't try them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globify-gitignore doesn't have any open issues or PRs, so I think it might just be "done" (working as expected) rather than not maintained? 3 years doesn't seem too long to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool work! I think long-term we'll need to migrate to the vfs (and I hope also adopt the globify-gitignore package) but short-term this is definitely a step forward from where we are now. Nicely done!
No blockers from me - just suggestions that we can do as followups IMO.
| }; | ||
| } | ||
|
|
||
| export const gitignoreFilter = createGitignoreFilter(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Refactor] There's no need for this to be module-level state, right? I think it could be moved to inside computeUseDefinitions. Then it'd be a little more resilient to things changing between calls in --watch / multi-run mode.
| beforeEach(() => { | ||
| fs.rmSync(integrationRoot, { force: true, recursive: true }); | ||
| fs.mkdirSync(integrationRoot, { recursive: true }); | ||
| vi.stubGlobal("process", { ...process, cwd: () => integrationRoot }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Refactor] computeUseDefinitions already calls process.cwd() as well. Eventually we're likely to need to make all these functions work with a custom cwd / receive one from the calling host. How about changing createGitignoreFilter to take in a cwd: string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think we need to mock it anymore.
| @@ -0,0 +1,62 @@ | |||
| import { makeAbsolute } from "@flint.fyi/utils"; | |||
| import ignore from "ignore"; | |||
| import fs from "node:fs"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1605 added a full linter host that we can use. It'll eventually be a must-have, to avoid node:fs access (e.g. for Flint running in a browser context). Do you have bandwidth to refactor this PR to have createGitignoreFilter take that in instead of calling to node:fs methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
globify-gitignore doesn't have any open issues or PRs, so I think it might just be "done" (working as expected) rather than not maintained? 3 years doesn't seem too long to me.
cspell.json
Outdated
| "tsgolint", | ||
| "twoslash", | ||
| "typelang", | ||
| "unstub", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems still need.
lishaduck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd held off approving to see if we cared about LinterHost, but if we're happy to leave that for a followup, then it LGTM 👍🏻
|
@kovsu marking as |
lishaduck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loving it! 😍
| log("Collecting files from %d use pattern(s)", configDefinition.use.length); | ||
|
|
||
| const allFilePaths = new Set<string>(); | ||
| const gitignoreFilter = createGitignoreFilter(process.cwd(), host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const gitignoreFilter = createGitignoreFilter(process.cwd(), host); | |
| const gitignoreFilter = createGitignoreFilter(host.getCurrentWorkingDirectory(), host); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process.cwd() -> /User/xxx
linterhost.getCurrentDirectory() -> /user/xxx
when i use path.relative(), it throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's a bug... uh... can you take a look why? It definitely shouldn't be lowercase 🤔
CC @auvred in case I'm wrong
| beforeEach(() => { | ||
| fs.rmSync(integrationRoot, { force: true, recursive: true }); | ||
| fs.mkdirSync(integrationRoot, { recursive: true }); | ||
| vi.stubGlobal("process", { ...process, cwd: () => integrationRoot }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think we need to mock it anymore.
| const ig = ignore(); | ||
| const visited = new Set(); | ||
| const rootDir = process.cwd(); | ||
| const rootDir = cwd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const rootDir = cwd; |
lishaduck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to leave figuring out the cwd thing for a followup. LGTM 👍🏻
PR Checklist
status: accepting prsOverview