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

chore(lint): add complexity rules to picasso #860

Merged
merged 2 commits into from
Nov 15, 2019

Conversation

OleksandrNechai
Copy link
Collaborator

@OleksandrNechai OleksandrNechai commented Nov 13, 2019

FX-592

As we do not have much experience in complexity I was inspired by airbnb rules and this post:airbnb/javascript#1758. Especially this comment:

For me, more better metric and more quality code you will get if you combine: max-params, max-statements, max-statements-per-line, max-nested-callbacks and max-depth.

Taken comment from here: airbnb/javascript#1089

     /** 
     * Ensure small functions and no nested/callback hell.
     * If you need more, consider extracting some logic.
     * Also you have cool destructuring feature, use it.
     */

Another article was used.

All these rules cause 26 errors in staff-portal and hundreds in picasso. Do not worry, most of them are in autogenerated Icons :-)

@OleksandrNechai OleksandrNechai requested a review from a team November 13, 2019 13:47
.eslintrc Outdated
"max-lines": [
"error",
{
"max": 150,
Copy link
Collaborator

Choose a reason for hiding this comment

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

some of them can be quite difficult to fix

Screenshot 2019-11-13 at 17 32 47

I would give a bit more freedom

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So weird now that you pointed that out, I started to get many more errors... Yes, you are probably right, will increase to 200 it's pretty common limit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe 300 as for the beginning? 😄

@OleksandrNechai OleksandrNechai self-assigned this Nov 13, 2019
@OleksandrNechai OleksandrNechai force-pushed the fx-592-complexity-rules-in-picasso branch from 47eb406 to 883f3b9 Compare November 13, 2019 21:13
@OleksandrNechai OleksandrNechai force-pushed the fx-592-complexity-rules-in-picasso branch from d24614b to d1ccbae Compare November 14, 2019 13:30
@toptal-devbot
Copy link
Collaborator

🎉 Last commit is successfully deployed 🎉

Demo is available on:

Your davinci-bot 🚀

* Also you have cool destructuring feature, use it.
*/
"complexity": ["warn", { "max": 5 }],
"max-params": ["warn", 3],
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably start with this, based on warnings we will see what to tweak and what to add to davinci

Copy link
Contributor

@havenchyk havenchyk left a comment

Choose a reason for hiding this comment

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

I'm fine with this change, let's try it out.

PS @OleksandrNechai did you fix formatting manually? :) it was done by prettier-standard

@OleksandrNechai OleksandrNechai merged commit 09e743b into master Nov 15, 2019
@OleksandrNechai OleksandrNechai deleted the fx-592-complexity-rules-in-picasso branch November 15, 2019 09:18
@toptal-devbot
Copy link
Collaborator

🎉 This PR is included in version 3.42.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.

4 participants