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

refactor: Update ESLint rulesets #61

Draft
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

Pathan-Amaankhan
Copy link
Member

@Pathan-Amaankhan Pathan-Amaankhan commented Feb 12, 2025

Note

This pull request (PR) constitutes Part 2 of the migration process for the subsequent PRs:

What

  • Update the ESLint rulesets and enforce stricter guidelines.

ESLint Changes

.eslintrc.cjs File

parser

ignorePatterns

  • ignorePatterns are sorted ascending.

rules

@typescript-eslint/naming-convention
  • Enforcing naming conventions helps keep the codebase consistent, and reduces overhead when thinking about how to name a variable.
@typescript-eslint/no-restricted-imports
  • Restricts use of classnames & lodash imports in TypeScript files.
dot-notation
  • Enforces dot notation on Objects. Setting allowKeywords option to false prevents using reserved keywords.
eslint-comments/require-description
  • Throws error if directive comments are used without description.
import/default
  • If a default import is requested, this rule will report if there is no default export in the imported module.
import/named
  • Verifies that all named imports are part of the set of named exports in the referenced module.
jest/expect-expect
  • Ensure that there is at least one expect call made in a test.
jsdoc/no-types
  • Allows adding type-definitions to @param & @return
jsdoc/require-param-type
  • Requires that each @param tag has a type value (within curly brackets).
jsdoc/require-returns-type
  • Requires that @returns tag has a type value (in curly brackets).
@stylistic/jsx/jsx-closing-tag-location
  • This rule checks all JSX multiline elements with children (non-self-closing) and verifies the location of the closing tag is aligned with the line containing the opening tag.
no-empty-function
  • Disallow empty functions
no-restricted-imports
  • Restricts use of classnames & lodash imports in JavaScript files.
no-restricted-syntax
  • Disallows specified syntax (Currently no syntax is restricted but as the project grows certain restrictions will be added as required in future versions)
prefer-destructuring
  • Forces de-structuring of arrays and objects.
react/jsx-boolean-value
react/jsx-curly-brace-presence
  • Prevents adding unnecessary curly braces in jsx children and props.

overrides

  • Self-documented, Please refer changelog.

packages/eslint-config/index.js File

plugins

The following new plugins have been added:

extends

plugin:eslint-comments/recommended
  • ESLint rulesets for directive comments.

Related Issue(s):

Testing Instructions

  • Run npm install && npm run lint.

Additional Info

Checklist

  • I have read the Contribution Guidelines.
  • My code is tested to the best of my abilities.
  • My code passes all lints (ESLint, tsc, prettier etc.).
  • My code has detailed inline documentation.
  • I have added unit tests to verify the code works as intended.
  • I have updated the project documentation accordingly.

Sorry, something went wrong.

@Pathan-Amaankhan Pathan-Amaankhan self-assigned this Feb 12, 2025
@Pathan-Amaankhan Pathan-Amaankhan marked this pull request as ready for review February 12, 2025 17:55
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still a lot of changes here, with no justification of what/why.

Might make sense to move these into multiple (atomic) PRs, or to put them in our internal config for now until we can have a real discussion about what rules make it into a public ruleset and why.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have migrated rulesets to internal config in d5767a3.

Please let me know if I should migrate the added plugins & extended rulesets also to the internal config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless you want to spend the extra justifying them (and the other preexisting ones that are included in @wordpress/eslint-plugin/recommended) ;-)

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

:-/ Same problem as the previous (now closed PRs): i.e. this is changes a lot of configs, but there's

  • no justification (inline or in the PR description) for the changes
  • too much strictness going into an ecosystem package (vs our internal code that we could theoretically change up).

@Pathan-Amaankhan
Copy link
Member Author

Pathan-Amaankhan commented Feb 13, 2025

no justification (inline or in the PR description) for the changes

Please do let me know if I should go ahead and update the PR description explaining each ruleset along with the reason why we added it.

Copy link
Collaborator

@justlevine justlevine left a comment

Choose a reason for hiding this comment

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

Not finding some rules:

eslint-comments/require-description
jsx-closing-tag-location

image

Comment on lines +36 to +47
'jsdoc/require-jsdoc': [
'error',
{
require: {
ArrowFunctionExpression: true,
ClassDeclaration: true,
ClassExpression: true,
FunctionExpression: true,
MethodDefinition: true,
},
},
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Call it a hunch, but I'm betting all the JSDoc type changes should be stripped out into their own PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure (Just doc rules) but we can do that also.

Please let me know and will create a separate PR.

Copy link

changeset-bot bot commented Feb 17, 2025

⚠️ No Changeset found

Latest commit: bf7eaf9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Pathan-Amaankhan Pathan-Amaankhan marked this pull request as draft February 20, 2025 11:22
@Pathan-Amaankhan
Copy link
Member Author

Pathan-Amaankhan commented Feb 20, 2025

We will be breaking down this PR into smaller PRs so that it is easier to fix eslint error we get after adding the ruleset.
Keeping this PR in draft state so that we don't loose the change reference. Once all the nuclear PRs are merged, we can close this PR.

--- Edit ---
PR Lists:

  1. refactor!: move ESLint overrides from package to local ruleset #86
  2. chore: make repository ESLint ruleset stricter (first pass) #89
  3. chore: cleanup ESLint rulesets #84
  4. feat: Add recommended eslint rules for comments #98
  5. chore: enforce dot-notation eslint rule #99
  6. chore: Add eslint-comments/require-description eslint rule #100 (Dependencies: chore: cleanup ESLint rulesets #84)
  7. feat: Add @stylistic/jsx/jsx-closing-tag-location eslint rule #101 (Dependencies: chore: cleanup ESLint rulesets #84)
  8. chore: Add jsdoc/require-param-type eslint rule #102
  9. feat: Add no-empty-function eslint rule #104

@justlevine
Copy link
Collaborator

@Pathan-Amaankhan is there anything left from this PR worth backporting or can we close?

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

Successfully merging this pull request may close these issues.

None yet

2 participants