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: add support for ignoring sync methods from certain locations #392

Merged
merged 16 commits into from
Feb 12, 2025

Conversation

RebeccaStevens
Copy link

Fixes #391

Adds the ability to ignore entire packages, or just a subset of functions from them along with other things.
This also resolves the issue of name clashes.

Example:

/* eslint n/no-sync: ["error", { ignores: [{ from: 'package', package: 'effect' }]}] */

import { Effect } from "effect"
const value = Effect.runSync(Effect.succeed(42))

This follows ts eslint's TypeOrValueSpecifier.

@RebeccaStevens RebeccaStevens force-pushed the expand-ignore branch 4 times, most recently from c399441 to 1781930 Compare November 28, 2024 12:00
@aladdin-add aladdin-add self-requested a review November 29, 2024 09:53
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! Sorry for the delay in reviewing 🙈

The code looks nice, and everything looks well thought through. I have a couple of minor questions :)

@RebeccaStevens
Copy link
Author

RebeccaStevens commented Jan 24, 2025

I'm not completely sure whether we want to include the last commit or not.
e554e55

You can see the intention of this commit by how it changes the tests.

Edit: I've updated the logic for this. It's much simpler now and actually works.

@RebeccaStevens
Copy link
Author

tests pass on my machine but not in CI...

@scagood

This comment was marked as outdated.

@RebeccaStevens
Copy link
Author

Failing tests issue resolved - I didn't commit a few files (the ones in the fixture node_modules dir)

Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

I like the approach and the code -- Thank you for the tests too!


Im going to leave this for a day or two to see if anyone else from @eslint-community/eslint-plugin-node want to review. But thank you for the contribution!

Copy link

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@aladdin-add aladdin-add merged commit 5544f20 into eslint-community:master Feb 12, 2025
28 of 29 checks passed
@benmccann
Copy link

It looks like the CI has been failing since this PR was merged making the latest release fail

@aladdin-add
Copy link

It should have nothing to do with this change - even though I switched to the previous commit it still fails.

let's discuss it in #409

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Expand no-sync's ignores option
4 participants