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

ESLint Config Migration: Add a src/types-specific rule #1025

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

filmaj
Copy link
Contributor

@filmaj filmaj commented Jul 29, 2021

Summary

This is a PR that should be merged into #1024 and incrementally addresses #842.

This change extends the proposed ESLint config to allow for type properties in src/types/**/*.ts files to be named in snake_case. Of the 1342 linting problems still to resolve in #1024, 830 of them are for the @typescript-eslint/naming-contention rule (specifically: about renaming these properties to be camelCase), and this PR drops the number of problems from 1342 down to 745.

Here is an example of the naming-conventions rule failing without the changes in this pull request:

/Users/fmaj/src/bolt-js/src/types/view/index.ts
   41:5  error  Type Property name `enterprise_id` must match one of the following formats: camelCase           @typescript-eslint/naming-convention
   42:5  error  Type Property name `enterprise_name` must match one of the following formats: camelCase         @typescript-eslint/naming-convention

That said, even with enabling this rule, turns out some type properties under src/types then fail as we use camelCase for them 😄:

/Users/fmaj/src/bolt-js/src/types/middleware.ts
  43:3  error  Type Property name `botToken` must match one of the following formats: snake_case      @typescript-eslint/naming-convention
  48:3  error  Type Property name `userToken` must match one of the following formats: snake_case     @typescript-eslint/naming-convention
  54:3  error  Type Property name `botId` must match one of the following formats: snake_case         @typescript-eslint/naming-convention
  59:3  error  Type Property name `botUserId` must match one of the following formats: snake_case     @typescript-eslint/naming-convention
  63:3  error  Type Property name `teamId` must match one of the following formats: snake_case        @typescript-eslint/naming-convention
  67:3  error  Type Property name `enterpriseId` must match one of the following formats: snake_case  @typescript-eslint/naming-convention

What does the team think? Do we want to enforce naming conventions for src/types? I am coming at this task from a place of ignorance; I don't know how the source "should" look like, so this is an attempt at formalizing whatever style already exists in the project as the canonical ESLint style. If this is not the direction we want to go, cool - I am happy to tackle renaming the type properties. Consider this PR a place to discuss and debate 😄

Requirements (place an x in each [ ])

@seratch
Copy link
Member

seratch commented Jul 30, 2021

@filmaj Thanks for flagging this!

I also have been aware of the inconsistency of the context object's camelCase naming in comparison with other data objects. But I'm not sure about the reasons why the team decided to use camelCase over snake_case for it (I was not yet a maintainer of this project at that time).

What does the team think? Do we want to enforce naming conventions for src/types?

Changing the names is not a small breaking change for existing apps and online resources by us and communities. For this reason, I agree that we should keep the names as-is. For the future, we can try to make property naming as consistent as possible.

@filmaj
Copy link
Contributor Author

filmaj commented Jul 30, 2021

@seratch thanks for the context! Agreed that breaking backwards compatibility to enforce lint rules does not seem worth it at all 😆

I will adjust the rules so that both snake_case and camelCase are accepted.

@filmaj filmaj force-pushed the rules-for-types-dir branch 2 times, most recently from 0aadb71 to 3eec381 Compare July 30, 2021 13:18
@filmaj filmaj merged commit 3841b6d into tslint-to-eslint Aug 4, 2021
@filmaj filmaj deleted the rules-for-types-dir branch August 4, 2021 18:39
@filmaj filmaj changed the title [WIP] ESLint Config Migration: Add a src/types-specific rule ESLint Config Migration: Add a src/types-specific rule Aug 5, 2021
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.

3 participants