Skip to content

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: TSESLint.RuleTester = ESLintRuleTester as any;

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 :)

@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.

3 participants