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

[code-infra] Replace all instances of e with event and add eslint rule #43866

Merged
merged 9 commits into from
Sep 30, 2024

Conversation

samuelsycamore
Copy link
Contributor

@samuelsycamore samuelsycamore commented Sep 23, 2024

@samuelsycamore samuelsycamore added the enhancement This is not a bug, nor a new feature label Sep 23, 2024
@mui-bot
Copy link

mui-bot commented Sep 23, 2024

Netlify deploy preview

https://deploy-preview-43866--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against e088fea

@oliviertassinari oliviertassinari changed the title Replace all instances of e with event [docs] Replace all instances of e with event Sep 24, 2024
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Sep 24, 2024
@Janpot
Copy link
Member

Janpot commented Sep 24, 2024

In #41968 I proposed to use the id-denylist rule instead for this purpose

@samuelsycamore
Copy link
Contributor Author

In #41968 I proposed to use the id-denylist rule instead for this purpose

Oh cool! I missed that conversation. I'll add that here then.

@samuelsycamore samuelsycamore added the scope: code-infra Specific to the core-infra product label Sep 24, 2024
@samuelsycamore samuelsycamore changed the title [docs] Replace all instances of e with event [code-infra] Replace all instances of e with event Sep 24, 2024
@samuelsycamore samuelsycamore changed the title [code-infra] Replace all instances of e with event [code-infra] Replace all instances of e with event and add eslint rule Sep 24, 2024
@Janpot
Copy link
Member

Janpot commented Sep 24, 2024

cc @LukasTy Just a heads up, this will generate new lint errors on X

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looks good, I would suggest, before merging this, to do the same exercise on the X repo. To avoid monorepo upgrade in X being blocked on code changes.
Did it for toolpad here

@oliviertassinari
Copy link
Member

I love to see this 👍

@JCQuintas
Copy link
Member

what about specifically targeting function/trycatch parameters?

In x there are some instances of valid use-cases for the e key inside an object, {e} eg: inside the date adapters

    'no-restricted-syntax': [
      'error',
      {
        selector: 'FunctionDeclaration > Identifier[name="e"]',
        message: 'Don\'t use "e" as an identifier, it\'s too easy to confuse with "c" or "o".',
      },
      {
        selector: 'ArrowFunctionExpression > Identifier[name="e"]',
        message: 'Don\'t use "e" as an identifier, it\'s too easy to confuse with "c" or "o".',
      },
      {
        selector: 'CatchClause > Identifier[name="e"]',
        message: 'Don\'t use "e" as an identifier, it\'s too easy to confuse with "c" or "o".',
      },
    ],

@Janpot
Copy link
Member

Janpot commented Sep 25, 2024

Some drawbacks of that approach is that

  • It's cumbersome to add more identifiers than just "e" ("err", "res", "t"), perhaps can be solved by generating those rules dynamically.
  • It misses cases such as
    import e from 'foo'
    
    const e = 'foo'
    but we could add two extra rules for these cases

Personally, If we have to add one or two ignore comments, I wouldn't bother complicating it too much. If it's more systemic, then it makes sense to me.

@JCQuintas
Copy link
Member

no-restricted-syntax

We can use regex and selectors to make it more generic. I just went with easy ones.

https://eslint.org/docs/latest/extend/selectors

@samuelsycamore
Copy link
Contributor Author

@JCQuintas I propose we merge this as-is now, and if/when the ignore comments become cumbersome then we should consider implementing the no-restricted-syntax rules.

@JCQuintas
Copy link
Member

@JCQuintas I propose we merge this as-is now, and if/when the ignore comments become cumbersome then we should consider implementing the no-restricted-syntax rules.

That works for me

@samuelsycamore samuelsycamore merged commit 371f7e2 into mui:master Sep 30, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation enhancement This is not a bug, nor a new feature scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants