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

[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 :crossed_fingers:

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

[aaron-m-edwards]

No it should come from the linters, linters are a common thing used on projects. We can't be expected to look over every students code constantly to enforce consistency in code. A linter is a tool that is there for this reason and we should take advantage of it.

Let's not do anything cruel to the students, I would rather get them use to good habits from day 1 rather than confusing them by changing half way through.

I think a better way to go about is to set up their editors correctly so it pulls in the eslint file and highlights the warnings/errors as they are typing the code rather than waiting for them to run it manually or when they fail the build.

As a side note, everyone should set up their favourite editor for eslint so we can feed this data in for our "This is the editor we are all using with these plugins" thing

Some of the people on my current team use the atom package eslint which seem to work well for them (I recommend ALE for vim8 users, but I doubt vim should be our recommended editor)

[alex-mitchem]

Just to clarify, I meant we have the linter set to show warning, with the trainers/POs encouraging them to eliminate any warnings their code produces. They're already learning a new language, do we really want to give them additional syntax constraints on day one? I can imagine it being frustrating to not be able to use code from tutorials or stack overflow because they don't follow the linter's rules.

I agree that setting up their editors to use lint would be the ideal solution, but that has it own issues associated with it.

As for being cruel, that might only be necessary if they decide not to follow the rules at all, and even then only for things that could be fixed reasonably easily.

For us though it's worth using really strict rules though, ideally any code we provide the students should be as close to a perfect coding example as possible.

[aaron-m-edwards]

We didn't have an issue with this last semester. Looking at the eslint.rc.json file from last semesters we used basically airbnb-base without any of the customisations we seem to have in the rewrite.

As for editors, please discuss here https://github.com/twlevelup/watch_edition_react/issues/21

[sinan-aumarah]

What is stopping students from removing the eslint config file or modifying it? nothing really. If they get sick of the errors/warnings they will most likely turn it off. I think we should really be focusing on the eslint rules that we want right now. Think of them as the base-code (framework) rules. Then maybe when we have a demo or two before we hold the event we can change the rules as we like and pick the ones most trainers will agree with.

[watsonarw]

What is stopping students from removing the eslint config file or modifying it?

git revert 😛

But in all seriousness, I've not seen a student change eslint config ever. We had students ask us how to change test coverage thresholds once, and we told them that they weren't allowed to and that they should write more tests. They generally listen when we tell them not to do things.

[aaron-m-edwards]

What is stopping students from removing the eslint config file or modifying it?

git revert 😛

I also do this when I see a commit that comes through without tests