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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: port lowercase-name to TypeScript #258

Merged
merged 4 commits into from May 12, 2019
Merged

chore: port lowercase-name to TypeScript #258

merged 4 commits into from May 12, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 10, 2019

For #256. Includes #257, so just review the second commit.

The diff looks bigger than it is since I had to rewrite all the helpers to include type guards. The implementation of the rule itself is identical.

I can extract the changes to index etc to a separate PR if wanted so this is focused on just the rule porting

@jeysal any help here would be greatly appreciated 馃檪

@SimenB SimenB requested a review from macklinu May 10, 2019 21:02

const { RuleTester } = require('eslint');
const rule = require('../lowercase-name');
const RuleTester: TSESLint.RuleTester = ESLintRuleTester as any;
Copy link
Member Author

Choose a reason for hiding this comment

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


const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
Copy link
Member Author

Choose a reason for hiding this comment

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

I find it odd they require this - even if the rule is written in TS, it might not necessarily only run with that parser. Might be needed for all I know 馃し鈥嶁檪

Copy link
Member

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

I glanced at the typing of the code (and ended up writing a wall of text on type guards 馃槅). @SimenB If you want me to look in any other specific places, feel free to tell me :)

src/rules/lowercase-name.ts Outdated Show resolved Hide resolved
src/rules/lowercase-name.ts Outdated Show resolved Hide resolved
src/rules/lowercase-name.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Member Author

SimenB commented May 12, 2019

@jeysal thanks for the review! I reimplemented this now with what I think is a better approach, with my own internal types that are more specific, and type guarding for them. I like this approach, could you take a look?

@jeysal
Copy link
Member

jeysal commented May 12, 2019

Typings look nice! 馃槄 (hell yeah, statically verify everything 馃挭)
The one missing line of coverage looks like it could be covered by having a callee that is not an identifier (e.g. member expression a.b())

@SimenB
Copy link
Member Author

SimenB commented May 12, 2019

Thanks @jeysal!

@SimenB SimenB changed the title chore: port lowercase-name.ts to TypeScript chore: port lowercase-name to TypeScript May 12, 2019
@SimenB SimenB merged commit ca2aa27 into master May 12, 2019
@SimenB SimenB deleted the typescript branch May 12, 2019 17:37
@SimenB SimenB mentioned this pull request May 12, 2019
35 tasks
@SimenB
Copy link
Member Author

SimenB commented May 22, 2019

馃帀 This PR is included in version 22.6.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants