Skip to content

fix(apps/oxlint): incorrect matching in .oxlintignore#7566

Merged
Boshen merged 2 commits intooxc-project:mainfrom
shulaoda:fix/oxlint-.oxlintignore
Dec 2, 2024
Merged

fix(apps/oxlint): incorrect matching in .oxlintignore#7566
Boshen merged 2 commits intooxc-project:mainfrom
shulaoda:fix/oxlint-.oxlintignore

Conversation

@shulaoda
Copy link
Copy Markdown
Contributor

@shulaoda shulaoda commented Dec 1, 2024

The issue was discovered while updating test cases in rolldown.

In the .oxlintignore file, we have:

tests/**

When running the command by lint-staged:

oxlint -c .oxlintrc.json --ignore-path=.oxlintignore --deny-warnings "tests/function/main.js"

The file main.js gets linted despite being ignored. This happens because, before using Gitignore::new, we converted paths to absolute paths, causing the pattern tests/** to not match D:/rolldown/tests/function/main.js correctly.

// append cwd to all paths
paths = paths
.into_iter()
.map(|x| {
let mut path_with_cwd = self.cwd.clone();
path_with_cwd.push(x);
path_with_cwd
})
.collect();
// The ignore crate whitelists explicit paths, but priority
// should be given to the ignore file. Many users lint
// automatically and pass a list of changed files explicitly.
// To accommodate this, unless `--no-ignore` is passed,
// pre-filter the paths.
if !paths.is_empty() && !ignore_options.no_ignore {
let (ignore, _err) = Gitignore::new(&ignore_options.ignore_path);
paths.retain(|p| if p.is_dir() { true } else { !ignore.matched(p, false).is_ignore() });
}

@graphite-app
Copy link
Copy Markdown
Contributor

graphite-app Bot commented Dec 1, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions Bot added A-cli Area - CLI C-bug Category - Bug labels Dec 1, 2024
Copy link
Copy Markdown
Member

@Boshen Boshen left a comment

Choose a reason for hiding this comment

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

Can we add a test for catching regressions?

@pumano
Copy link
Copy Markdown
Contributor

pumano commented Dec 1, 2024

Today I test it too, and got same problem but with "ignorePatterns" from config

@shulaoda shulaoda requested a review from Boshen December 1, 2024 14:13
@shulaoda shulaoda force-pushed the fix/oxlint-.oxlintignore branch from 0698aed to fca6c44 Compare December 1, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants