replace code-review checklist items with lint rules

[pixelzoom]

Noted while doing the code review for Wave Interference.

Most (all?) of the "Math Libraries" section of the code-review checklist could (should?) be replaced with lint rule(s).

Math Libraries

• Check that dot.Util.roundSymmetric is used instead of Math.round. Math.round does not treat positive and negative numbers symmetrically, see fix nearest-neighbor rounding in Util.toFixed dot#35 (comment).
• DOT/Util.toFixed or DOT/Util.toFixedNumber should be used instead of toFixed. JavaScript's toFixed is notoriously buggy. Behavior differs depending on browser, because the spec doesn't specify whether to round or floor.
• Check that random numbers are generated using phet.joist.random, and are doing so after modules are declared (non-statically). Including, but not limited to:
• No usage of Math.random
• No usage of _.shuffle
• No usage of _.sample
• No usage of _.random
• No usage of new Random()

Assigning to @ariel-phet to prioritize and assign.

[pixelzoom]

Ditto for the "IE11" section:

IE11

• No usage of Number.parseInt()
• No usage of Array.prototype.find

[jonathanolson]

No usage of new Random()

That seems like a rule for simulations. There are places in common code (for generating a reproducible fuzzing stream) where this is needed. snapshot-comparison also reaches in and does random = new iframe.contentWindow.phet.dot.Random( { seed: 2.3 } );.

Many of those rules also don't particularly apply to other code. Do we have to figure out how to add a custom PRNG library to Node.js/testing/wrapper code to avoid Math.random()?

There are also common code places where Math.random() is used because that common code can't depend on joist (where phet.joist.random is declared)

So for each of the potential rules, I'd think carefully about which different scopes they would be desired in, e.g. "lint for just sim code", "lint for browser code", "lint for all code", etc.

[pixelzoom]

I did say "Most (all?)", so I'm fine if we don't include Random (or others) in a lint rule. But the majority of them can be addressed via lint. And anything to shorten the code-review checklist is imo a "win".

[zepumph]

Since work done in phetsims/tasks#972 on separating out different sets of lint rules, we can likely decide to just enforce something like "no random" as a lint rule for simulations only.

[pixelzoom]

Unfortunately we can't just add these to bad-text.js. E.g. "toFixed" would flag both the forbidden and recommended function names.

[pixelzoom]

In recent code review of Wave Interference, Math.round was flagged but ultimately allowed for performance reasons, as documented in LatticeCanvasNode:

// ImageData.data is Uint8ClampedArray.  Use Math.round instead of Util.roundSymmetric
// because performance is critical and all numbers are non-negative.
const offset = 4 * m;
data[ offset ] = Math.round( r );
data[ offset + 1 ] = Math.round( g );
data[ offset + 2 ] = Math.round( b );

Would we require these cases to use /* eslint-disable */?

[zepumph]

If we did, I think the best way would be to say // eslint-disable-line {{round-specific-lint-rule-name}}

[zepumph]

In dev meeting it was decided that the above can be added to the "bad-text" lint rule. But since that rule applies to all lintable files, we will create a new eslint config that only applies to sims, and copy the bad-text lint rule and call the copy bad-sim-text, that way we can still use Math.random in node or something.

Steps to continue:

• Create sim_eslintrc.js that extends the base config
• Point all es5 sims to use that instead of the base config.
• Point es6 sim config to extend this new sim config.
• create bad-sim-text.js lint rule by copying bad-text.js, add above texts to this
• Determine if any current bad-texts just apply to sims.

[zepumph]

@jessegreenberg I'll investigate that in #737, for now it is commented out, when I fix it I will make sure that those usages are fixed, so don't worry.

[ariel-phet]

@pixelzoom mind reviewing? If you don't have time please assign back to me...most other devs are tied up until beginning of Feb @zepumph so if @pixelzoom is not available, it will just have to wait a little bit.

[pixelzoom]

Looks good expect for commented code related to toFixed that is being tracked in #737. I can't think of any other code-review items that should be added here. Back to @zepumph in case there's anything else to do. Feel free to close.

[zepumph]

I just realized that these items have not been removed from the code review checklist. Wasn't that the goal of this issue?

Or do we still want to double check on them just in case?

[pixelzoom]

I removed the check-list items (the entire "Math Libraries" section) in the above commit.

[zepumph]

Sounds good. Note that until #737 is fixed, we are missing one check. And also note that though this does a pretty good job, it doesn't ensure that random numbers are being generated with phet.joist.random

[pixelzoom]

... until #737 is fixed, we are missing one check

Reopening. It took me some digging, but I guess you're referring to uses of built-in toFixed. I restored that checklist item in the above commit. If this was incorrect, please restore whatever "missing one check" refers to.

[pixelzoom]

I noted in #737 (comment) that the check-list item for toFixed should be removed when that issue is resolved. If there's nothing else to do here, please close.

[zepumph]

Thanks! That all sounds great.