Eslint Rules

[aaron-m-edwards]

At the moment we using airbnb as a base with a bunch of rules that have been disabled (see current file here)

[aaron-m-edwards]

Ray did mention that he does not agree with all of the rules in the airbnb style and I am inclined to agree, however I think some of the rules we have tuned off are useful.

[watsonarw]

Normally, I'd say turn off a few rules, and leave it up to developer best judgement, but given this app is for very junior developers, I'm inclined to think that stricter rules is better because it leads to more consistency, and more consistency makes it easy to pick up someone else's code and understand what's happening.

Airbnb's eslint rules are quite strict, and I think probably the right level for something like this.

[aaron-m-edwards]

Things I would like to turn back on

• arrow-parens (prevents accidental "if (a => 4) {}")
• new-cap (I just like to tell the difference between a class instance and variable, but don't care too much)
• no-return-assign (Lets keep functions pure if possible)
• jsx-quotes: [1, "prefer-double"] (This is more of a personal one, but I don't care that much)
• react/forbid-prop-types (use shape or arrayOf instead)
• react/jsx-filename-extension (easier to see jsx vs js, we are not consistent in this now)
• react/prefer-stateless-function (also remove the babel transformer)
• import/extensions (reduce confusion)
• import/no-extraneous-dependencies (prevent people from saving dependencies in wrong places - we will need to exclude tests from this rule)
• import/no-unresolved (small check for typos in import statements)

[sinan-aumarah]

@aaron-m-edwards enabled most of the ones you mentioned and fixed the project :)

react/prefer-stateless-function: if I turn it on then I have to change the components that don't use props or state to pure functions. Do we want that? it will look quite inconsistent 😕

[aaron-m-edwards]

React does a lot of optimisations on stateless functions. If we want to live without it sure, but then we will have to turn all the stateless function components to extend Component

[sinan-aumarah]

Agreed. I'm just not sure if that will confuse students or not. I might enable it and turn the ones with no state to pure functions 🤞

[aaron-m-edwards]

So I think if we do want to keep consistency change all of the stateless functions under /app (I wouldn't worry as much about /framework) to extend Component. If we do want to teach good React habits (Although I still say teaching coding is not really the goal) then keep the rule.

[watsonarw]

I don't think there's an eslint rule that prohibits functional components. At least with the react/prefer-stateless-function rule, all stateless components are functions, and we can enforce that consistency. Without the rule, we'll probably end up with a mix anyway, at least this way there's a reason to it. We go from one way to do stateful components and two ways to do stateless components, to only one way to do each.

[raytung]

In my experience, most teams would probably have features like

• make the clock look prettier, maybe some formatting to the time, add AM PM, timezone etc
• add multiple pages that are actually menus, in which you can navigate to another page to display something.
• Display a list, probably from a JSON file.
• And if it's a high functioning team with multiple capable devs, there might be a page where you click some button to change how the page looks.

prefer-stateless-function do discourage people from modifying the state of the rendered component, which I think is a good thing because of the skew towards read more than write operations in our levelup app, as seen in the most common features above.

Things I'm more interested in :

• Rules listed by Aaron
• The no-param-reassign rule gives us the most bang-for-buck in teaching pure functions, which is turned on in the airbnb rules anyway.
• The no-magic-number rule
• The no-undefined rule
• The no-shadow rule
• The no-redeclare rule
• The no-unused-var rule
• .eslintrc styling - prefer { off , warn , error } over { 0 , 1 , 2}
• Not sure whether multiple nested currying / partial functions is a good idea.

    const a = () => () => {}; // 1 level of partial function is ok

   /* 
    * more than 1 level starts to impede readability, especially for people with
    * less coding experience
    * maybe it's just my inexperience in functional languages though
    */
    const b = (c) => (d) => (e) => (f) => {};

EDIT: Think I just spammed everybody with my 1000000 edits. xoxo

[alex-mitchem]

I think prefered-stateless-function is a good idea. It will force the students to write simple solutions and stop them getting carried away. It will also help keep us focused on teaching development skills, not react skills.

On a side note, can we set the levels of each rule? I was getting compile errors because I'd used double quotes in my import statements instead of single and because I didn't space my closing brackets. Seems a bit excessive.

[sinan-aumarah]

Cool. I'll enable it then.

@AMitchemTW yeah we can. Just instead of 'error' make it 'warn'

[watsonarw]

I still think excessive is good. Warnings will be ignored, errors can't be, and I'd prefer to just not have the rule at all instead of setting it to warning.

Having had to explain the importance of things like indentation when the next person comes along and tries to understand it, or for finding that missing semicolon, or the extra } on multiple occasions, I'd prefer to be strict with our linting rules.

In saying that, " vs ' is something that I really don't care about, so I'd be OK with turning that particular rule off completely. (Although it does prevent edit wars with people constantly changing it back and forth).

[aaron-m-edwards]

+1 for leaving most as error. Errors will fail the build and prevent the dev server from showing, warnings will not and probably be ignored. I don't care if we go double or single quotes but I would still put in the rule for one or the other (If I had to choose I would go single).

@AMitchemTW if you run npm lint -- --fix it will automatically fix things like quotes and spaces padding around { and }

[alex-mitchem]

I agree on being strict, I just think it should come from the trainers not the code. I think there's value in teaching them self discipline with regards to their code, they should be following the lint rules because they want to write consistent, good quality code, not because we're forcing them. Think of it like teaching a child manners.

On a side note, we could always be cruel and leave the rules as warnings the first few weeks, then set them to errors (ie "PO changes their mind"). Seems like the kind of thing they'd do in TWU.

@aaron-m-edwards Wish I'd known that yesterday, boy was that frustrating. I think we should also try and keep this from students, let them do it the hard way.