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

@shopify/eslint-plugin: merge typescript naming convention rules #247

Merged
merged 1 commit into from
May 10, 2021

Conversation

brendanrygus
Copy link
Contributor

@brendanrygus brendanrygus commented May 10, 2021

Description

While upgrading to v40.2.2, we noticed some changes in the @typescript-eslint/naming-convention rule.
Some of the recent changes around enforcing T prefixes for generic types might have loosened up other checks.

These previously invalid declarations all became valid in ts/tsx files:

type myCamelCasedType = 'ok'
const my_snake_cased_var = 'ok, ruby'
const object = {
  my_backend_enum_key: 'yup cool',
  MY_SCREAMING_KEYS: 'whatever'
}

Type of change

This PR merges the two @typescript-eslint/naming-convention rules together to restore the previous config.

  • @shopify/eslint-plugin Patch: Bug (non-breaking change which fixes an issue)

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish a new version for these changes)

@@ -131,6 +131,19 @@ module.exports = {
selector: 'typeLike',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the --print-config CLI arg to log out the eslint rules our repo used before the recent changes

I confirmed that the rules for typescript-eslint/naming-convention matched those in this file, so merging the two rule sets together seems like our best bet

    "@typescript-eslint/naming-convention": [
      "error",
      {
        "selector": "default",
        "format": [
          "camelCase",
          "PascalCase",
          "UPPER_CASE"
        ],
        "leadingUnderscore": "allow",
        "trailingUnderscore": "allow"
      },
      {
        "selector": "default",
        "filter": {
          "match": true,
          "regex": "^(__|UNSAFE_).+$"
        },
        "format": null
      },
      {
        "selector": "typeLike",
        "format": [
          "PascalCase"
        ]
      }
    ],

prefix: ['T'],
},
{
selector: 'interface',
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Contributor

@dahukish dahukish left a comment

Choose a reason for hiding this comment

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

This makes sense. Seems like the old rule was being overwritten hence the revert on what was valid. No 🎩 yet.

@brendanrygus brendanrygus marked this pull request as ready for review May 10, 2021 17:03
@@ -7,6 +7,10 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

<!-- ## Unreleased -->

### Changed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @vsumner !

@brendanrygus
Copy link
Contributor Author

Tophatting against my local app, and it looks like eslint has Strong Opinions again 😄
Screen Shot 2021-05-10 at 12 44 19 PM

@brendanrygus brendanrygus merged commit 0e1857c into main May 10, 2021
@brendanrygus brendanrygus deleted the restore-ts-naming-convention branch May 10, 2021 17:25
@shopify-shipit shopify-shipit bot temporarily deployed to production May 10, 2021 17:29 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants