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

Update linting rules #351

Merged
merged 4 commits into from Jul 25, 2019
Merged

Update linting rules #351

merged 4 commits into from Jul 25, 2019

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Jul 24, 2019

Did this via editor to prevent having to shelf changes locally just to checkout master 馃槀

Feel free to push up prettier or other formatting changes if required.

These are the rules I recommend having, feel free to ask questions :)

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jul 24, 2019

Ugh except I just realised that travis will fail b/c of eslint now, so I'll have to check it out locally to fix.

I'll do that sometime soon.

Also might as well chuck in no-else-return

@G-Rath
Copy link
Collaborator Author

G-Rath commented Jul 24, 2019

I've removed @typescript-eslint/no-unnecessary-type-assertion & @typescript-eslint/no-unnecessary-qualifier b/c while very cool & useful, they require type information, which means linting doubles in time.

It's not the end of the world, but something I figured you guys could decide since you're paying for the pipeline :P

@@ -25,6 +25,10 @@ module.exports = {
es6: true,
},
rules: {
'@typescript-eslint/array-type': ['error', 'array-simple'],
Copy link
Member

Choose a reason for hiding this comment

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

In Jest we have this set to generic for consistency

Copy link
Member

Choose a reason for hiding this comment

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

consistency with what? (I don't care either way, and since this is fixable it really doesn't matter what we go with, as long as we lint for one or the other 馃檪)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My two-cents: I've found array-simple to look rather nice, and gives you the best of both worlds in terms of readability vs extra typing/characters/line length.

Just for the record, here's the difference:

// 'generic' - always use "Array", never "[]"
declare const arr: Array<string | number>;
declare const arr: Array<MyObj<string>>;
declare const arr: Array<string>;

// 'array-simple' - always use '[]', unless the generic type is "complex" (uses generics, union, etc)
declare const arr: Array<string | number>;
declare const arr: Array<MyObj<string>>;
declare const arr: string[];

// 'simple' - always use `[]`, never `Array`
declare const arr: (string | number)[];
declare const arr: MyObj<string>[];
declare const arr: string[];

I use to use generic, but got sick of having to go to the start of the type everytime I wanted to make thing like a string an array - w/ array-simple you can just keep typing when doing quick speedy type definitions.

Copy link
Member

Choose a reason for hiding this comment

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

Just thought it'd be nice to have a code base that is easy to get into if you've already worked on Jest itself. Don't care much either, I always write it in array and let it autofix anyway 馃槃

@SimenB SimenB merged commit 1992b3d into jest-community:master Jul 25, 2019
@G-Rath G-Rath deleted the patch-2 branch July 25, 2019 11:20
@SimenB
Copy link
Member

SimenB commented Jul 25, 2019

馃帀 This PR is included in version 22.13.7 馃帀

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

3 participants