Skip to content

feat: add meta.defaultOptions#17656

Merged
mdjermanovic merged 62 commits into
eslint:mainfrom
JoshuaKGoldberg:rule-meta-default-options
Nov 15, 2024
Merged

feat: add meta.defaultOptions#17656
mdjermanovic merged 62 commits into
eslint:mainfrom
JoshuaKGoldberg:rule-meta-default-options

Conversation

@JoshuaKGoldberg

@JoshuaKGoldberg JoshuaKGoldberg commented Oct 16, 2023

Copy link
Copy Markdown
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Augments linting to factor in a new optional meta.defaultOptions. If a rule describes default options this way, any user-provided options are merged on top of the default options.

These defaults are applied by a new deepMergeArrays utility in:

  • ConfigValidator
  • Linter in two places:
    • For configs: by augmenting the existing getRuleOptions helper
    • For inline comments in flat config: with a new getRuleOptionsInline helper
      • Inline comments in legacy configs go through RuleValidator instead
  • RuleValidator

Is there anything you'd like reviewers to focus on?

Co-authored-by: Milos Djermanovic milos.djermanovic@gmail.com

@eslint-github-bot

This comment was marked as outdated.

@netlify

netlify Bot commented Oct 16, 2023

Copy link
Copy Markdown

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit a3bf8b6
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/670695cb45e9610008fd897e
😎 Deploy Preview https://deploy-preview-17656--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@eslint-github-bot

This comment was marked as outdated.

@eslint-github-bot

This comment was marked as outdated.

@github-actions

github-actions Bot commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions Bot added the Stale label Nov 5, 2023
Comment thread tests/lib/linter/deep-merge.js Outdated
@github-actions github-actions Bot removed the Stale label Nov 6, 2023
@github-actions

This comment was marked as outdated.

@github-actions github-actions Bot added the Stale label Nov 16, 2023
@Rec0iL99 Rec0iL99 removed the Stale label Nov 17, 2023
@Rec0iL99

Copy link
Copy Markdown
Member

Not stale. @JoshuaKGoldberg there's a review comment for you to look into. Thanks.

@eslint-github-bot

This comment was marked as outdated.

@eslint-github-bot

Copy link
Copy Markdown

Hi @JoshuaKGoldberg!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@eslint-github-bot

This comment was marked as outdated.

@eslint-github-bot

This comment was marked as outdated.

@eslint-github-bot

This comment was marked as outdated.

@eslint-github-bot

This comment was marked as outdated.

@github-actions

github-actions Bot commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

@github-actions github-actions Bot added the Stale label Dec 1, 2023
Comment thread lib/config/config.js Outdated
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
mdjermanovic
mdjermanovic previously approved these changes Nov 13, 2024

@mdjermanovic mdjermanovic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks! Leaving open for @nzakas to verify.

I'll just convert it to draft to prevent accidental merging until we commit #17656 (comment).

@mdjermanovic mdjermanovic marked this pull request as draft November 13, 2024 14:53
nzakas
nzakas previously approved these changes Nov 13, 2024

@nzakas nzakas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mdjermanovic mdjermanovic dismissed stale reviews from nzakas and themself via b2742ba November 15, 2024 16:36
@mdjermanovic mdjermanovic marked this pull request as ready for review November 15, 2024 16:39
@mdjermanovic mdjermanovic merged commit 2edc0e2 into eslint:main Nov 15, 2024
@JoshuaKGoldberg JoshuaKGoldberg deleted the rule-meta-default-options branch November 15, 2024 17:04
voxpelli added a commit to voxpelli/eslint that referenced this pull request Nov 25, 2024
Seems like eslint#17656 introduced an unintended change in `no-warnings-comments.js` where before it roughly looked like:

```js
var configuration = context.options[0] || {};
var decoration = [...configuration.decoration || []].join("");
```

And afterwards it looks like:

```js
const [{ decoration, location, terms: warningTerms }] = context.options;
```

Which unintentionally(?) removes the `context.options[0] || {}` default, which eg. causes `eslint-plugin-unicorn@<15.0.1` to fail (see sindresorhus/eslint-plugin-unicorn#2497)

Since this happened in a minor `eslint` release but the fix for `eslint-plugin-unicorn` is only available as a patch to its latest major, this is causing me some issues.

My guess is that you won't be wanting to merge this though as `eslint-plugin-unicorn` is [extending that rule](https://github.com/sindresorhus/eslint-plugin-unicorn/blob/8b7c5fcf5c9743db4afde0bb9c90cc51782d47ef/rules/utils/get-builtin-rule.js#L4) through:

```js
require('eslint/use-at-your-own-risk').builtinRules.get('no-warning-comments')
```

And as discussed in eslint#19013 you don't consider that a good practise:

> It's in unstable APIs because rule APIs are indeed not stable

> Core rules are not intended to extended any you do so at your own risk

Perhaps eslint#19013 or similar can suggest an alternative for eg. `eslint-plugin-unicorn` to avoid this happening again in the future, but my guess is you consider such suggestions to be out of scope of `eslint` and as such to not be something you ant to get involved in.

Here's a PR fixing the regression at least, you decide if you want to accept it or reject it 🤷‍♂️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint rule Relates to ESLint's core rules

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

8 participants