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

Prioritize ignored paths when linting #1878

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

clarkf
Copy link
Contributor

@clarkf clarkf commented Jan 3, 2024

As discussed in #1124

A common workflow is running oxlint automatically via something like lint-staged, which passes all changed file paths explicitly as CLI arguments. The ignore crate auto-whitelists explicit paths, overriding anything defined in an ignore file, leading to files that should be ignored causing CI and VC hooks to fail.

eslint avoids this issue by giving precedence to .eslintignore unless you pass --no-ignore.

Since ignore doesn't really give much control over this, the most effective solution seems to be filtering. Once we've got a list of files to be linted, we can compare each against the .eslintignore and see if it really truly should be linted.

Unfortunately, this is sort of a naive solution. Ignored directories will still be walked, then filtered out after the fact. Changing this behavior at the walker level would affect the formatter, which probably isn't desirable. (edit: The performance impact has been mostly resolved.)

@github-actions github-actions bot added the A-cli Area - CLI label Jan 3, 2024
Copy link

codspeed-hq bot commented Jan 3, 2024

CodSpeed Performance Report

Merging #1878 will not alter performance

Comparing clarkf:lint-no-ignore (3655c92) with main (45a7985)

Summary

✅ 14 untouched benchmarks

Copy link
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.

This is going to cause performance regression I think, given that no_ignore = false by default and thousands of paths collected from Walk.

Should we only trigger this code path when there are file paths passed from the command line?

For a more granular control, should we introduce a --lint-staged flag to explicitly handle all the cases for lint staged?

@Boshen
Copy link
Member

Boshen commented Jan 3, 2024

I like the overall direction of this PR, it's just that eslint + lint-staged caused so much configuration confusion and wasted time for people, I have a feeling first-class support (via a cli flag) will be better than some random combinations of configuration options.

What do you think?

@clarkf
Copy link
Contributor Author

clarkf commented Jan 3, 2024

You're right that this introduces a performance regression, it definitely does. In most cases, the paths will be iterated a second time, filtered, and re-Veced. I think the pathological case might be overstated, though. For the overhead to be truly significant, the user would have to have passed a parent path to a significant number of ignored paths. Think something like oxlint node_modules/. This could be avoided by pushing the ignore filtering up into Walk, probably.

Adding the check for explicit paths is an easy win, though. I'll definitely do that, which should avoid the overhead for the common case.

...eslint + lint-staged caused so much configuration confusion and wasted time for people, I have a feeling first-class support (via a cli flag) will be better than some random combinations of configuration options.

The confusion is definitely real, and ultimately I'd really defer to your judgement. My preference would be to stick with this direction and not implement git-specific logic. For what it's worth, this behavior (.{tool}ignore file taking precedence over provided paths) is how most of the common tools in this corner of the world (git, eslint, prettier) work.

@Boshen
Copy link
Member

Boshen commented Jan 3, 2024

Shall we try the following logic:

for each provided path, 
  if the path is a file (not a directory)
    check against ignore logic

FYI oxlint is designed to work on huge monorepos, with 10,000+ files being the norm.

A common workflow is running `oxlint` automatically via something like
lint-staged, which passes all changed file paths explicitly as CLI
arguments. The ignore crate auto-whitelists explicit paths, overriding
anything defined in an ignore file, leading to files that should be
ignored causing CI and VC hooks to fail.

eslint avoids this issue by giving precedence to .eslintignore unless
you pass `--no-ignore`.

Since `ignore` doesn't really give much control over this, the most
effective solution seems to be pre-filtering. Each provided path can be
considered against the ignore file, which should keep performance nearly
the untouched.

Signed-off-by: Clark Fischer <clark.fischer@gmail.com>
@clarkf
Copy link
Contributor Author

clarkf commented Jan 3, 2024

I've approximated the requested logic, I think. Notably, it is now possible to pre-filter all paths, skipping linting altogether.

There's an outstanding question of user feedback. At no point during this process is the user informed that paths or filtered. eslint yields linter warnings, which doesn't seem perfect, but better than silence.

@Boshen Boshen enabled auto-merge (squash) January 4, 2024 03:43
@Boshen Boshen merged commit d88d76f into oxc-project:main Jan 4, 2024
15 checks passed
@Boshen
Copy link
Member

Boshen commented Jan 4, 2024

Thank you!

There's an outstanding question of user feedback. At no point during this process is the user informed that paths or filtered. eslint yields linter warnings, which doesn't seem perfect, but better than silence.

We can have another issue / PR for this.

@clarkf clarkf deleted the lint-no-ignore branch January 4, 2024 14:16
Brooooooklyn referenced this pull request in toeverything/AFFiNE Jan 7, 2024
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [oxlint](https://oxc-project.github.io) ([source](https://togithub.com/oxc-project/oxc/tree/HEAD/npm/oxlint)) | [`0.0.22` -> `0.1.2`](https://renovatebot.com/diffs/npm/oxlint/0.0.22/0.1.2) | [![age](https://developer.mend.io/api/mc/badges/age/npm/oxlint/0.1.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/oxlint/0.1.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/oxlint/0.0.22/0.1.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/oxlint/0.0.22/0.1.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>oxc-project/oxc (oxlint)</summary>

### [`v0.1.2`](https://togithub.com/oxc-project/oxc/releases/tag/oxlint_v0.1.2): oxlint v0.1.2

[Compare Source](https://togithub.com/oxc-project/oxc/compare/821cc8e9c7cfb326ff546483bb2a32d12e018e4c...4a9e0c4bf4179bf5839b690be3690163ff00e2ef)

#### Try it out!

-   Run `npx --yes oxlint@latest` from your terminal
-   Install the Vscode extension https://marketplace.visualstudio.com/items?itemName=oxc.oxc-vscode
-   Read the [usage guide](https://oxc-project.github.io/docs/guide/usage/linter.html)

#### Svelte support

`<script>` tag is linted by default.

#### Features

-   feat(linter): <script> part of svelte file by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/1918](https://togithub.com/oxc-project/oxc/pull/1918)
-   feat(linter): disable no-unused-labels for svelte by [@&#8203;Boshen](https://togithub.com/Boshen) in [https://github.com/oxc-project/oxc/pull/1919](https://togithub.com/oxc-project/oxc/pull/1919)

### Fixes

-   fix(linter): change no-var to restriction [`bb6128b`](https://togithub.com/oxc-project/oxc/commit/bb6128b)
-   chore: add some useful informantion log by [@&#8203;IWANABETHATGUY](https://togithub.com/IWANABETHATGUY) in [https://github.com/oxc-project/oxc/pull/1912](https://togithub.com/oxc-project/oxc/pull/1912)
-   fix(linter) fix eslint config for filename case by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1913](https://togithub.com/oxc-project/oxc/pull/1913)

**Full Changelog**: oxc-project/oxc@oxlint_v0.1.1...oxlint_v0.1.2

### [`v0.1.1`](https://togithub.com/oxc-project/oxc/releases/tag/oxlint_v0.1.1): oxlint v0.1.1

[Compare Source](https://togithub.com/oxc-project/oxc/compare/v0.1.0...821cc8e9c7cfb326ff546483bb2a32d12e018e4c)

#### Try it out!

-   Run `npx --yes oxlint@latest` from your terminal
-   Install the Vscode extension https://marketplace.visualstudio.com/items?itemName=oxc.oxc-vscode
-   Read the [usage guide](https://oxc-project.github.io/docs/guide/usage/linter.html)

#### Vue support

`<script>` and `<script setup>` are linted by default.

#### Astro support

Frontmatter component script `---` and all `<script>` tags are linted by default.

#### Configuration files (experimental)

`-c ./eslintrc.json` will use the `rules` field for rule configuration, as documented in [ESLint's documentation](https://eslint.org/docs/latest/use/configure/rules#using-configuration-files).

Unfortunately, only the `json` format is supported right now.

The `extends` field will not take effect; normal `-D` and `-A` flags still apply.

#### New Rules

##### Correctness

-   deepscan: bad object literal comparison by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1844](https://togithub.com/oxc-project/oxc/pull/1844)
-   oxc: erasing op by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1834](https://togithub.com/oxc-project/oxc/pull/1834)
-   oxc: only used in recursion by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1833](https://togithub.com/oxc-project/oxc/pull/1833)
-   eslint: no irregular whitespace by [@&#8203;DeividAlmeida](https://togithub.com/DeividAlmeida) in [https://github.com/oxc-project/oxc/pull/1877](https://togithub.com/oxc-project/oxc/pull/1877)
-   eslint: no-unused-private-class-members rule by [@&#8203;Dunqing](https://togithub.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/1820](https://togithub.com/oxc-project/oxc/pull/1820)
-   eslint: no-var by [@&#8203;zhangrunzhao](https://togithub.com/zhangrunzhao) in [https://github.com/oxc-project/oxc/pull/1890](https://togithub.com/oxc-project/oxc/pull/1890)
-   eslint-plugin-react: jsx-no-undef for by [@&#8203;XantreGodlike](https://togithub.com/XantreGodlike) in [https://github.com/oxc-project/oxc/pull/1862](https://togithub.com/oxc-project/oxc/pull/1862)
-   eslint-plugin-jsx-a11y: aria-role by [@&#8203;msdlisper](https://togithub.com/msdlisper) in [https://github.com/oxc-project/oxc/pull/1849](https://togithub.com/oxc-project/oxc/pull/1849)
-   eslint-plugin-jsx-a11y: lang by [@&#8203;msdlisper](https://togithub.com/msdlisper) in [https://github.com/oxc-project/oxc/pull/1812](https://togithub.com/oxc-project/oxc/pull/1812)
-   eslint-plugin-jsx-a11y: media-has-caption by [@&#8203;poteboy](https://togithub.com/poteboy) in [https://github.com/oxc-project/oxc/pull/1822](https://togithub.com/oxc-project/oxc/pull/1822)
-   eslint-plugin-jsx-a11y: mouse-events-have-key-events(correctness) by [@&#8203;Ken-HH24](https://togithub.com/Ken-HH24) in [https://github.com/oxc-project/oxc/pull/1867](https://togithub.com/oxc-project/oxc/pull/1867)
-   eslint-plugin-jsx-a11y: prefer-tag-over-role rule by [@&#8203;yossydev](https://togithub.com/yossydev) in [https://github.com/oxc-project/oxc/pull/1831](https://togithub.com/oxc-project/oxc/pull/1831)
-   eslint-plugin-jsx-a11y: aria-unsupported-elements by [@&#8203;re-taro](https://togithub.com/re-taro) in [https://github.com/oxc-project/oxc/pull/1855](https://togithub.com/oxc-project/oxc/pull/1855)
-   eslint-plugin-jsx-a11y: html_has_lang by [@&#8203;msdlisper](https://togithub.com/msdlisper) in [https://github.com/oxc-project/oxc/pull/1843](https://togithub.com/oxc-project/oxc/pull/1843)

##### Suspicious

-   oxc: approx constant by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1818](https://togithub.com/oxc-project/oxc/pull/1818)
-   oxc: misrefactored assign op by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1832](https://togithub.com/oxc-project/oxc/pull/1832)

##### Restriction

-   react: button has type by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1785](https://togithub.com/oxc-project/oxc/pull/1785)
-   unicorn: prefer modern math apis by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1620](https://togithub.com/oxc-project/oxc/pull/1620)

#### Fixes

-   fix(linter): ignore anonymous functional components in arrays for eslint-plugin-react(jsx-key) by [@&#8203;maurice](https://togithub.com/maurice) in [https://github.com/oxc-project/oxc/pull/1858](https://togithub.com/oxc-project/oxc/pull/1858)
-   Prioritize ignored paths when linting by [@&#8203;clarkf](https://togithub.com/clarkf) in [https://github.com/oxc-project/oxc/pull/1878](https://togithub.com/oxc-project/oxc/pull/1878)
-   feat(linter): handle more cases for `const-comparisons` by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1817](https://togithub.com/oxc-project/oxc/pull/1817)
-   feat(semantic): allow reserved keyword defined in ts module block by [@&#8203;Dunqing](https://togithub.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/1907](https://togithub.com/oxc-project/oxc/pull/1907)
-   fix(parser): error on source larger than 4 GiB by [@&#8203;overlookmotel](https://togithub.com/overlookmotel) in [https://github.com/oxc-project/oxc/pull/1860](https://togithub.com/oxc-project/oxc/pull/1860)
-   fix(parser): fix incorrectly identified directives by [@&#8203;overlookmotel](https://togithub.com/overlookmotel) in [https://github.com/oxc-project/oxc/pull/1885](https://togithub.com/oxc-project/oxc/pull/1885)
-   fix(parser): terminate parsing if an EmptyParenthesizedExpression error occurs by [@&#8203;Dunqing](https://togithub.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/1874](https://togithub.com/oxc-project/oxc/pull/1874)
-   fix(semantic): remove duplicate errors in ModuleDeclaration::ImportDeclaration by [@&#8203;Dunqing](https://togithub.com/Dunqing) in [https://github.com/oxc-project/oxc/pull/1846](https://togithub.com/oxc-project/oxc/pull/1846)
-   perf(linter): reduce the number of diagnostics for no_sparse_arrays by [@&#8203;camc314](https://togithub.com/camc314) in [https://github.com/oxc-project/oxc/pull/1895](https://togithub.com/oxc-project/oxc/pull/1895)

#### New Contributors

-   [@&#8203;maurice](https://togithub.com/maurice) made their first contribution in [https://github.com/oxc-project/oxc/pull/1858](https://togithub.com/oxc-project/oxc/pull/1858)
-   [@&#8203;re-taro](https://togithub.com/re-taro) made their first contribution in [https://github.com/oxc-project/oxc/pull/1855](https://togithub.com/oxc-project/oxc/pull/1855)
-   [@&#8203;DeividAlmeida](https://togithub.com/DeividAlmeida) made their first contribution in [https://github.com/oxc-project/oxc/pull/1835](https://togithub.com/oxc-project/oxc/pull/1835)
-   [@&#8203;XantreGodlike](https://togithub.com/XantreGodlike) made their first contribution in [https://github.com/oxc-project/oxc/pull/1862](https://togithub.com/oxc-project/oxc/pull/1862)
-   [@&#8203;Qix-](https://togithub.com/Qix-) made their first contribution in [https://github.com/oxc-project/oxc/pull/1861](https://togithub.com/oxc-project/oxc/pull/1861)
-   [@&#8203;yossydev](https://togithub.com/yossydev) made their first contribution in [https://github.com/oxc-project/oxc/pull/1831](https://togithub.com/oxc-project/oxc/pull/1831)
-   [@&#8203;clarkf](https://togithub.com/clarkf) made their first contribution in [https://github.com/oxc-project/oxc/pull/1878](https://togithub.com/oxc-project/oxc/pull/1878)
-   [@&#8203;zhangrunzhao](https://togithub.com/zhangrunzhao) made their first contribution in [https://github.com/oxc-project/oxc/pull/1890](https://togithub.com/oxc-project/oxc/pull/1890)

**Full Changelog**: oxc-project/oxc@oxlint_v0.0.22...oxlint_v0.1.1

### [`v0.1.0`](https://togithub.com/oxc-project/oxc/releases/tag/v0.1.0): CLI v0.1.0 Ezno Type Checker

[Compare Source](https://togithub.com/oxc-project/oxc/compare/a1accdca7f83694a6ea520d5cbfd090ea5dd271a...v0.1.0)

`npx oxidation-compiler@latest check ./test.ts`

![image](https://togithub.com/Boshen/oxc/assets/1430279/c7308395-1856-43fa-b4b8-b239886ec259)

#### New Contributors

-   [@&#8203;anonrig](https://togithub.com/anonrig) made their first contribution in [https://github.com/Boshen/oxc/pull/388](https://togithub.com/Boshen/oxc/pull/388)
-   [@&#8203;kaleidawave](https://togithub.com/kaleidawave) made their first contribution in [https://github.com/Boshen/oxc/pull/413](https://togithub.com/Boshen/oxc/pull/413)

**Full Changelog**: https://github.com/Boshen/oxc/compare/v0.0.7...

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/toeverything/AFFiNE).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyMS4wIiwidGFyZ2V0QnJhbmNoIjoiY2FuYXJ5In0=-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area - CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants