Skip to content

Autogenerate configs from rule files #187

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

Closed
nickserv opened this issue Jun 23, 2020 · 12 comments · Fixed by #358
Closed

Autogenerate configs from rule files #187

nickserv opened this issue Jun 23, 2020 · 12 comments · Fixed by #358
Assignees
Labels
pinned Pinned for different reasons. Issues with this label won't be flagged as stale by stalebot released

Comments

@nickserv
Copy link
Member

See extended discussion in #182

@nickserv nickserv self-assigned this Jun 23, 2020
@stale
Copy link

stale bot commented Apr 11, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Apr 11, 2021
@stale stale bot closed this as completed Apr 18, 2021
@nickserv
Copy link
Member Author

Is this something you'd still like help with?

@Belco90
Copy link
Member

Belco90 commented Apr 25, 2021

Hey @nickmccurdy! Yes, for sure. I'll go back to this definitely when couple of extra rules are released. Anyway I had a couple of ideas around this, so I'll try to put them here later to see what we can do. Thanks!

@MichaelDeBoey
Copy link
Member

I've been playing around with this idea too lately.

My idea was to add some sort of supportedTestingFrameworks key in the meta docs that's an array of strings and that's been read in order to know if it's in a certain config (for that specific testing framework).

@MichaelDeBoey MichaelDeBoey reopened this Apr 25, 2021
@stale stale bot removed the wontfix This will not be worked on label Apr 25, 2021
@Belco90
Copy link
Member

Belco90 commented Apr 25, 2021

Oh sorry, I didn't realized it was closed by stale-bot!

My idea was to add some sort of supportedTestingFrameworks key in the meta docs that's an array of strings and that's been read in order to know if it's in a certain config (for that specific testing framework).

Yes, this is one of the things I had in mind, but we don't need to add an extra key for it: we can reuse recommended from rule meta.docs. We can reuse few things from meta.docs actually, and extend it so we have something like:

const ruleConfig = {
  meta: {
    docs: {
      description: string, // existing - small description of the rule --> used for rule doc md file + README rules table
      category: 'Possible Errors' | 'Best Practices' | 'Style Guidelines', // reused - categories for our plugin rather than ESLint categories --> used for rule doc md file
      recommended: string[], // reused - list of framework presets where this rule is enabled rather than ESLint recommended or not --> used for rule doc md file + README rules table
      onlyFor: string[] // new - frameworks this rule applies exclusively --> used for rule doc md file
    },
  },
};

When "rule doc md file" means a new small table at the top of each rule doc file for indicating category, recommended presets, and if it's exclusive for some frameworks.

What do you think?

@MichaelDeBoey
Copy link
Member

I would keep recommended as it's intended to use: to mark it as error, warn or off.
That way we can have ruleA warn and ruleB error in our configs if we ever want to do this in the future.

For category, I would keep them all as 'Best Practices' as a lot of other ESLint plugins are keeping the ESLint categories too and don't reuse them in the context of their own plugin.

I like the idea of having the distinction of saying "this framework supports it" and "we should include it in the config for this framework", but I'm not sure about the naming.

So what I would propose to do is:

const ruleConfig = {
  meta: {
    docs: {
      // existing
      description: string,
       // existing
      category: 'Possible Errors' | 'Best Practices' | 'Style Guidelines',
      // existing
      recommended: string,
      // list of frameworks that are supported by this rule, like your `onlyFor` so we explicitly say what frameworks are supported
      supportedTestingFrameworks: ('dom' | 'angular' | 'react' | 'vue')[],
      // list of framework configs this rule is included
      recommendedFor: ('dom' | 'angular' | 'react' | 'vue')[],
    },
  },
};

Showing the supported frameworks and what config the rule is included in in the docs MD of the rule sounds like a great idea too.

@Belco90
Copy link
Member

Belco90 commented Apr 25, 2021

I'm confused about recommended key now. In types coming from @typescript-eslint/experimental-utils is indicated as false | "warning" | "error" but in ESLint docs is indicated as boolean.

Anyway, both ways are meant to indicate if the rule has to be included in the recommended preset or not. We don't have such a preset though, but a list of recommended ones per framework, so having meta.docs.recommended as a single value means nothing for us. Instead, we can rewrite it as a list or even better as an object where keys are Shareable Config names and values are false | "warning" | "error" to match the types you mentioned, which are mode detailed.

With this idea we wouldn't need recommendedFor but reuse recommended. Everything else would be the same form your last suggestion.

For category, I would keep them all as 'Best Practices' as a lot of other ESLint plugins are keeping the ESLint categories too and don't reuse them in the context of their own plugin.

That's the point of my suggestion: since we are not interested in default ESLint categories, we override it with our own ('Possible Errors' | 'Best Practices' | 'Style Guidelines' which is different from ESLint categories), so we use it to indicate the category at the top of each rule doc. This would help users to know how subjective/objective the rules are and to which area they apply.

@MichaelDeBoey
Copy link
Member

Instead, we can rewrite it as a list or even better as an object where keys are Shareable Config names and values are false | "warning" | "error" to match the types you mentioned, which are mode detailed.

With this idea we wouldn't need recommendedFor but reuse recommended.

That's indeed even a better idea 👍, since we can then have

  • 1 less key
  • different recommended settings for each testing framework

That's the point of my suggestion: since we are not interested in default ESLint categories, we override it with our own ('Possible Errors' | 'Best Practices' | 'Style Guidelines' which is different from ESLint categories), so we use it to indicate the category at the top of each rule doc. This would help users to know how subjective/objective the rules are and to which area they apply.

I'm not 100% convinced about this one tbh.
We could do this in a separate PR though if the team decides this is a better way.

@Belco90
Copy link
Member

Belco90 commented Apr 25, 2021

We can debate about the categories themselves for sure! Another idea I had for that was just defining categories as priorities level as in Common mistakes with React Testing Library post, so we would have category: "low" | "medium" | "high" which would be reflected on each rule doc file.

Anyway, this was just an idea to use category for something useful (as it's not used at all at the moment), and give to the users some context around area or importance of the rule.

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Apr 25, 2021

Instead, we can rewrite it as a list or even better as an object where keys are Shareable Config names and values are false | "warning" | "error" to match the types you mentioned, which are mode detailed.

With this idea we wouldn't need recommendedFor but reuse recommended.

Thinking about this a bit more: we could even keep it as it is and make it recommended for all frameworks (with the specified value) if it's not an object.

@Belco90 Belco90 added the pinned Pinned for different reasons. Issues with this label won't be flagged as stale by stalebot label Apr 25, 2021
@Belco90
Copy link
Member

Belco90 commented Apr 25, 2021

I'd avoid enabling the rule for all frameworks if recommended defined as true since that would enable them automatically for new frameworks.

For instance: rule await-async-utils is enabled for all framework presets at the moment and we introduce a new compatible framework (let's say "svelte") which by any reason shouldn't have this rule enabled in the preset:

  • recommended: true would enable it for this new framework preset implicitly
  • recommended: { dom: true, ...} wouldn't enable it for this new framework preset automatically, and we would need to add it manually so we are forced to review rule per rule explicitly

In cases like this, explicit is better than implicit.

@github-actions
Copy link

github-actions bot commented May 3, 2021

🎉 This issue has been resolved in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Pinned for different reasons. Issues with this label won't be flagged as stale by stalebot released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants