Commented out lint rules

[zepumph]

While working on cleaning up phetsims/tasks#972, I saw

    //    'require-jsdoc': [
    //      2
    //    ],

And wondered if we wanted to investigate this at all, or just delete it. @samreid do you have any insight?

[samreid]

I flip that rule on temporarily sometimes during code reviews. But it is now deprecated and ESLint is dropping support for it, see https://eslint.org/docs/rules/valid-jsdoc

[zepumph]

I also see:

 //    'no-use-before-define': [
    //      2
    //    ],
    // TODO: This may be good to have
    //    'new-cap': [
    //      2
    //    ],


    // disallow trailing whitespace at the end of lines (fixable)
    //    'no-trailing-spaces': 2,

Mainly I want to know if we should do one of three things:

1. Delete these
2. Uncomment them
3. Mark them with // USE THIS DURING CODE REVIEW which I see another lint rule doing.

[samreid]

I've pulled the commented out rules into a dedicated file, and added it to the code review checklist. @zepumph can you please review?

[zepumph]

Looks great!

[pixelzoom]

Reopening.

Slack developer channel:

Chris Malley [4:30 PM]
A new code-review checklist item is “Does linting with “sim_es6_eslintrc_review.js” reveal any problems?” What is the point of this? I’m having all kinds of problems with this lint file. By convention we define private functions after the class definition, and lint is flagging all of these as “used before defined” errors. I also have dozens of “exceeds the maximum line length of 120" where I’m a char or two over, and intended to exercise my “developer discretion” . Also a dozen or so “no-trailing-spaces” errors that WebStorm can’t find when I request a reformat. (edited)

So for my first experience with this… Lots of noise, lots of busy-work, zero benefit so far.

Michael Kauzmann [4:32 PM]
I am inclined to agree with you. I ran it for GFLB and basically just reformatted line comments for 10 minutes.
I think it has helped @samreid before, and perhaps he would know more.

[pixelzoom]

In my single experience with this (phetsims/gas-properties#114), it identified no real problems, and only generated busy work. Some rules don't even match PhET conventions. 'no-use-before-define' is generating a lot of noise because it's PhET convention to define private functions after the class definition. 'max-len' 120 is not a hard rule, it's up to developer discretion. And 'no-trailing-spaces': 2, kept me busy for 20 minutes reformatting files and hunting for spaces that WebStorm can't find (like spaces after the define function).

I'd like to hear other developers' experiences.

[samreid]

I found numerous occurrences of trailing whitespace in comments--those seem tolerable. I tried changing the rule to:

'no-trailing-spaces': ["error", { "ignoreComments": true }],

and saw 348 occurrences of trailing whitespace in code (not in comments). For example:

When I applied the IDEA formatting rule, these extraneous whitespaces were removed. So this seems like a one way to detect when the IDEA formatter is not being used.

I'm happy to get rid of the no-use-before-define rule. Not sure about the 120 limit. Maybe we would say that 120 is a good rule of thumb (developer discretion can go over), but 180 is a hard limit? Or something like that. new-cap seems to be a useful rule, but not sure what others think.

I don't feel too strongly about this, but just wanted to throw in 2 cents.

[zepumph]

'no-trailing-spaces' perhaps should be included in all lint rules, since it is a sign that formatting is not being applied. Perhaps people would be opposed though.

[samreid]

In #765 @jessegreenberg said:

One of the steps in the code review checklist is

Does linting with "sim_es6_eslintrc_review.js" reveal any problems that should be fixed? (change eslintConfig in package.json and run grunt lint)

When I ran it for gravity-force-lab-basics it identified a number of lint issues mostly related to line length. We determined that these don't need to be fixed. Should the line length check be changed for sim_es6_eslintrc_review?

It also flagged DerivedPropertyIO( NumberIO ):

A function with a name starting with an uppercase letter should only be used as a constructor (new-cap)

I am not sure I understand the purpose of this check - since it is for es6 are we hoping that all es6 sims will pass these rules? Or is this just a way to flag things that might be problems.

@samreid and @zepumph are listed as authors, what do you think?

[samreid]

The *_review.js rules were intended to flag things that might be problems. But it sounds this supplemental eslint file is more trouble than it is worth. I recommend we move any desirable rules to the main eslint file and delete this one.

[pixelzoom]

+1 for moving desirable rules to the main eslint file and deleting sim_es6_eslintrc_review.js.

[jbphet]

Another +1 for moving desirable rules to the main eslint file. I am currently working through the code review checklist for "Gas Properties", and the item containing "Does linting with "sim_es6_eslintrc_review.js" reveal any problems that should be fixed?" feels like a waste of time. There is a rule no-use-before-define that is flagging a number of things that seem to be exactly as they should be, so at the very least, that rule should be immediately removed.

[samreid]

Tagging for developer meeting to confirm exactly which rules we want to move to the main eslint file.

I recommend to move the following rules:

'no-trailing-spaces': 2,

    // specify the maximum length of a line in your program
    'max-len': [
      2,
      // Our convention is to limit to 120 with "use developer discretion", but this is the hard limit (or use
      // eslint-disable)
      160
    ],

Note that build-a-molecule/js/model/data/otherMoleculesData.js will need a file-oriented disable.

[samreid]

eslint review file removed, trailing whitespace rule added, offending files addressed. Will monitor CT for a bit and close if all is clear.

[samreid]

CT seems clear, closing.