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

Change Request: Attach default options as an object to rules #17448

Open
1 task done
JoshuaKGoldberg opened this issue Aug 7, 2023 · 8 comments
Open
1 task done

Change Request: Attach default options as an object to rules #17448

JoshuaKGoldberg opened this issue Aug 7, 2023 · 8 comments
Assignees
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Aug 7, 2023

ESLint version

v8.46.0

What problem do you want to solve?

Right now, there's no programmatic way to determine what any given rule's default options are. Many rules define functions like normalizeOptions (array-bracket-newline), array-element-newline, object-curly-newline) or parseOptions (no-implicit-coercion, use-before-define) that apply various, subtly different value defaults.

As a result, the only user-facing way to determine a rule's default options is to read the docs. But the docs themselves aren't consistent in phrasing. For example:

Furthermore, there's also no way for tooling to determine the default options for any rule. Which is making it hard for us in typescript-eslint to automate our docs pages for extended rules (typescript-eslint/typescript-eslint#6841 (comment)).

What do you think is the correct solution?

In typescript-eslint, we added a defaultOptions property on rules. It's visible in the Custom Rules guide, which suggests users wrap rule creation with a createRule:

import { ESLintUtils } from '@typescript-eslint/utils';

const createRule = ESLintUtils.RuleCreator(
  name => `https://example.com/rule/${name}`,
);

export const rule = createRule({
  create(context) { /* ... */ },
  defaultOptions: [ { /* ... */ } ],
  meta: { /* ... */ },
  name: "..."
});

Under the hood, createRule wraps create to fill in defaultOptions for any option not provided by the ESLint config. You can see the exact logic here: https://github.com/typescript-eslint/typescript-eslint/blob/9c17395d7cd6da7e2db59f9bde2c81264a33da97/packages/utils/src/eslint-utils/applyDefault.ts. It's a deep merge, not a shallow Object.assign.

Would ESLint core be interested in adding an equivalent to this logic? And/or, would you like an RFC written for it?

Participation

  • I am willing to submit a pull request for this change.

Additional comments

I don't believe this would be a breaking change the way #14709 is. Making the new field optional means rules can opt into it if they please.

@JoshuaKGoldberg JoshuaKGoldberg added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Aug 7, 2023
@bmish
Copy link
Sponsor Member

bmish commented Aug 9, 2023

I'd love to be able to programmatically tell what the default options are for a rule, also for automatic documentation purposes.

It should be noted that some defaults can be documented inside the JSON Schema (meta.schema), but I'm not sure this is sufficient to represent all defaults (I think there was a past discussion about this, but can't remember where).

@nzakas
Copy link
Member

nzakas commented Aug 9, 2023

I think this is worth exploring and I'd be open to an RFC. The original intent was for the JSON schema to be the source of truth for default options (indeed we used this when --init scanned files to figure out which options to best suggest) but it ended up being more complicated than it was worth.

If we were to implement something like this, some things I'd expect to see:

  1. If default options are specified, then ESLint should merge those options with any explicit options specified by the user. That way, we can avoid checking for the presence of certain options inside the rule itself.
  2. defaultOptions inside of meta rather than at the top level.
  3. RuleTester should validate the default options against the rule options schema.

@nzakas nzakas added the needs design Important details about this change need to be discussed label Aug 9, 2023
@bradzacher
Copy link
Contributor

bradzacher commented Aug 10, 2023

It should be noted that some defaults can be documented inside the JSON Schema (meta.schema), but I'm not sure this is sufficient to represent all defaults

It's definitely possible to do this - though personally I don't like it because it's technically possible to do things like {type: 'number', default: 'hello'} which compile fine and will even work fine if you pass the option! It will only error if you skip the value and the default is used.

const validate = ajv.compile({
  type: 'object',
  properties: { foo: { type: 'number', default: 'hello' } },
});

const x = { foo: 1 };
validate(x); // -> true

const y = {};
validate(y); // -> false -> "Invalid: data/foo must be number"
console.log(y); // -> { foo: 'hello' };

Additionally from a type-safety perspective - it gets pretty hairy trying to safely type the default value. For simple cases like above - sure - easy, but for more complex cases (arrays / objects) it's mostly not possible.

Which was the reason we went with the separate defaultOptions prop in the typescript-eslint utilities as it allowed us to simply take the user's defined option type and use it for the default - easy validation!

@github-actions
Copy link

github-actions bot commented Sep 9, 2023

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Sep 9, 2023
@JoshuaKGoldberg
Copy link
Contributor Author

I still plan on writing an RFC Soon™️.

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Sep 18, 2023

@github-actions
Copy link

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 18, 2023
@nzakas
Copy link
Member

nzakas commented Oct 19, 2023

RFC is open, so not stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed
Projects
Status: Implementing
Development

No branches or pull requests

6 participants