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.

Sign up for GitHub

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

Jump to bottom

chore: enable ESLint's recommended JS rules #5282

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

chore: enable ESLint's recommended JS rules #5282

wants to merge 5 commits into from

Conversation

JoshuaKGoldberg
Copy link
Member

PR Checklist

Overview

Fixes all the no-unused-vars complaints, and suppresses the rules that would be more intricate to fix.

@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Mar 10, 2025
Open
5 tasks
JoshuaKGoldberg
JoshuaKGoldberg commented Mar 10, 2025
View reviewed changes
test/unit/runnable.spec.js
spy();
done();
}, 50);
}, 10);
Copy link
Member Author

@JoshuaKGoldberg JoshuaKGoldberg Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I tweaked this to have a bit smaller of a total setTimeout time.

Copy link
Member

Choose a reason for hiding this comment

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

Everything else looks good so I'm happy to approve, but I'm too new to comment on this one, hoping someone else can weigh in to confirm no breaking changes or anything!

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review March 10, 2025 20:59
@JoshuaKGoldberg JoshuaKGoldberg requested a review from a team March 10, 2025 20:59
mark-wiemer
mark-wiemer approved these changes Mar 15, 2025
View reviewed changes
test/unit/runnable.spec.js
spy();
done();
}, 50);
}, 10);
Copy link
Member

Choose a reason for hiding this comment

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

Everything else looks good so I'm happy to approve, but I'm too new to comment on this one, hoping someone else can weigh in to confirm no breaking changes or anything!

voxpelli
voxpelli requested changes Mar 17, 2025
View reviewed changes
Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

I think we should go with neostandard instead of continue to customize our own. Mocha used to be a standard linted project

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Mar 17, 2025 •
edited
Loading

@voxpelli I don't see why we need to block this in the meantime. Are there specific lint rules from the ESLint recommended set that you don't think we should enable?

I personally don't like big conglomeration projects like neostandard. I find them to conflate logical rules with stylistic ones, and obfuscate the differences by presenting a single unified config area. I might not be correct and am certainly open to the discussion - but I think there's going to be back-and-forth on whether to use neostandard.

If anything, I would think enabling the ESLint recommended rules would help onboard to neostandard, no? Either:

  • (my understanding) ESLint recommended is a subset of neostandard, so this will reduce the cost of moving to neostandard
  • ESLint recommended isn't a subset of neostandard, so these rules are presumably additional usefulness

@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Mar 17, 2025
Open
@voxpelli
Copy link
Member

I don't see the reluctance to move to neostandard – both me and @Uzlopak was pro that in #5281 (comment) and we both use it in our other projects such as fastify

As such: Why should we spend time setting up different rules and possibly diverging mocha from what it was prior to #5060 ? Isn't that just a waste of time that may in turn cause even more waste of time?

I personally don't like big conglomeration projects like neostandard. I find them to conflate logical rules with stylistic ones

Not true with neostandard – it has a noStyle option that opts out from style rules and the style rules are clearly separated from the rest. But since we don't use Prettier it can be good to do what eg. Fastify does and actually use the style rules.

ESLint recommended is a subset of neostandard, so this will reduce the cost of moving to neostandard

There is no relation between ESLint recommended and neostandard at all – any subset / superset is completely coincidental – I do not believe ESLint recommended is a subset

@JoshuaKGoldberg
Copy link
Member Author

Why should we spend time setting up different rules and possibly diverging mocha from what it was prior to #5060 ? Isn't that just a waste of time that may in turn cause even more waste of time?

Heh, this confuses me. Are you saying there are rules in ESLint's recommended set that you wouldn't want enabled? I'm not clear on how this answers #5282 (comment) - what work do you see as likely needing to be undone if+when Mocha switches to neostandard?

Especially given:

There is no relation between ESLint recommended and neostandard at all – any subset / superset is completely coincidental – I do not believe ESLint recommended is a subset

(ACK, thanks - I had misinterpreted. Filed neostandard/neostandard#273)

JoshuaKGoldberg
JoshuaKGoldberg commented Mar 18, 2025
View reviewed changes
eslint.config.js
@@ -20,6 +20,8 @@ module.exports = [
sourceType: 'script'
},
rules: {
'no-empty': 'off',
'no-redeclare': 'off',
'no-var': 'off',
Copy link
Member Author

Choose a reason for hiding this comment

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

This would be a TODO for a subsequent PR, to keep the diff small.

Copy link
Member

Choose a reason for hiding this comment

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

And no-var for sure!

@JoshuaKGoldberg JoshuaKGoldberg added the status: blocked Waiting for something else to be resolved label Mar 19, 2025
@JoshuaKGoldberg
Copy link
Member Author

Marking as blocked per discussion in #5301.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft March 19, 2025 12:38
@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Mar 19, 2025
Closed
@voxpelli voxpelli self-requested a review March 19, 2025 13:36
voxpelli
voxpelli reviewed Mar 19, 2025
View reviewed changes
Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

Can't spend more time on this, either create a PR of your own if there's any interest in neostandard or just merge this and disregard that.

voxpelli
voxpelli approved these changes Mar 19, 2025
View reviewed changes
Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

I guess I have to approve to get rid of the blocker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mark-wiemer mark-wiemer mark-wiemer approved these changes

@voxpelli voxpelli voxpelli approved these changes

Assignees
No one assigned
Labels
status: blocked Waiting for something else to be resolved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🛠 Repo: Enable ESLint's recommended rules for JS
3 participants
@JoshuaKGoldberg @voxpelli @mark-wiemer