Proposed eslint rule additions

[mramato]

I did a pass on all available rules on http://eslint.org/docs/rules/ and picked out ones that we probably want in Cesium. We'll want to test these out one by one to make sure the rule isn't already handled by some of the other rules we are already using. We can turn a bunch of these on at once, assuming they don't require a bunch of code changes. Rules that do require a bunch of code changes should be done separately. I didn't include any style rules since they will be part of #5369. I also didn't include rules that were likely never to come up (for example explicitly disallowing eval).

Things we definitely want (almost all of these were part of jsHint and long when moved to eslint)

• block-scoped-var
• default-case
• no-alert
• no-floating-decimal
• no-implicit-globals
• no-loop-func
• no-use-before-define

Things we probably want (Similar guidelines already exist in our documentation)

• no-else-return
• no-undef-init
• no-sequences
• no-unused-expressions

Node only (only apply to other Cesium ecosystem projects)

• global-require
• no-buffer-constructor
• no-new-require

Still want to evaluate, but level of effort not worth it right now

• no-shadow
• valid-jsdoc (may require lots of cleanup but well worth it)

[mramato]

If anyone has any thoughts on the above rules, or recommend additional rules that we should look into, please speak up. I'm hoping to have these rules added sometime over the next few weeks otherwise.

[ottaviohartman]

@mramato can I get started on adding some of these to Cesium?

[pjcozzi]

Things we definitely want (almost all of these were part of jsHint and long when moved to eslint)

👍

Things we probably want (Similar guidelines already exist in our documentation)

👍, except

class-methods-use-this

Does this apply to Cesium yet since it doesn't use class anywhere?

no-sequences

Generally, I would say yes, but I'm not sure we should commit to it since this allows for some nice graphics code like http://www.realtimerendering.com/blog/looping-through-polygon-edges/

Perhaps just enable it and see how many exceptions the current code base needs. If it is only a few, just stay with it.

Things we may want (cool ideas that might be overkill)
complexity

👎 This will be hard to reasonably enforce throughout the codebase, especially in areas of high algorithmic intensity. I expect there will be little return here and lots of frustration.

Node only (only apply to other Cesium ecosystem projects)

👍

Finally, please update the Coding Guide to be include anything new here. Throughout the Coding Guide, it would be useful to link to the specific ESLint doc as it often has more details and motivation than our guide.

[ottaviohartman]

I've just been playing around to see how breaking these would be. no-use-before-define is the biggest I've tested with >900 errors.

[ottaviohartman]

Many PR's open, which include these rules:

• guard-for-in
• no-caller
• no-floating-decimal
• no-new
• no-undef-init
• default-case
• no-alert
• no-loop-func
• no-implicit-globals
• no-use-before-define

[ottaviohartman]

consistent-return is proving difficult to implement. https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Globe.js#L384, and many other functions rely on returning either a value or undefined.

Should I enable the rule and silence the errors inline?

[pjcozzi]

Should I enable the rule and silence the errors inline?

Probably, unless there are dozens of these cases. We may come up with ideas to clean things up in the pull request.

[mramato]

Does this apply to Cesium yet since it doesn't use class anywhere?

I might have assumed it does since class is just a keyword, but I removed it from the list anyway after trying it out and seeing no results.

Generally, I would say yes, but I'm not sure we should commit to it since this allows for some nice graphics code like http://www.realtimerendering.com/blog/looping-through-polygon-edges/

The rule has a special case for for loops (which I agree is a common and often desired use of sequences). So this rule is exactly what we do in our own code already and will allow for the code you linked to.

[mramato]

I removed rules we either agreed worry worth it or were not what we ultimately wanted. I also moved no-shadow and valid-jsdoc to a different category because both require tons of changes. If I were starting a brand new code base, I would use them, but the way our code is right now isn't worth it until we have more time and decide we really want it.

[pjcozzi]

All sounds good.

[ottaviohartman]

@mramato Pretty sure it's ready to be bumped to 2.0!

[mramato]

@mramato Pretty sure it's ready to be bumped to 2.0!

Open a PR to enable the 3 node rules, it most likely won't require any Cesium code changes (but update CHANGES.md to mention them), also bump the version. Once that's done, we'll merge and deploy and then we can update all of the other repositories.

[ottaviohartman]

Ok! See #5540

[mramato]

2.0.0 has been published. Thanks @omh1280!