Closed samreid closed 3 years ago
I'm not going to get to this anytime soon. unassigning.
@samreid and I spent a long time over in #150 going through rules that had to do with formatting. I'm unsure how much more I would work on this. @samreid would you like to close this?
We reviewed this in https://github.com/phetsims/phet-info/issues/150 and https://github.com/phetsims/phet-info/issues/156 and made a lot of progress, particularly regarding formatting. It still may be valuable to review more broadly the rest of the ESLint rules. Perhaps I'll add this to my list at low priority.
Tagging to touch base at developer meeting.
There are some rules that seem like they should be on, such as array-callback-return
. If you call array.map( ()=>{})
but do not return a value in the arrow function, it seems problematic.
Also consistent-return
flags functions like this one in capacitor-lab-basics where it sometimes returns and sometimes falls through to inadvertently return undefined
(there are 79 cases failing this rule):
( capacitance, voltage, circuitConnection ) => {
if ( circuitConnection ) {
let charge = capacitance * voltage;
// Force an underflow to zero below the threshold for stability
if ( Math.abs( charge ) < CLBConstants.MIN_PLATE_CHARGE ) {
charge = 0;
}
return charge;
}
}
The main question for developer meeting is how to proceed on issues like this (identified as TODOs in the eslint files in chipper/eslint/), some options are:
Or we could start with (2) and then move to (3) or (1).
You mentioned that there are 79 failing cases for consistent-return
. What other rules are you proposing to enable, and what's the total number of failing cases? I would need that info to choose one of the options suggested in the previous comment.
'object-shorthand' // TODO: 448 fails
'prefer-arrow-callback' // TODO: 9 fails
'prefer-rest-params' // TODO: 2 fails
'prefer-template' // TODO: 99 fails
'no-async-promise-executor' // TODO: 4 fails
'require-atomic-updates' // TODO: 6 fails
'array-callback-return' // TODO: 14 fails
'consistent-return' // TODO: 79 fails
'default-param-last' // TODO: 1 failure
'no-constructor-return' // TODO 10 fails
'no-eval' // TODO: 2 fails
'no-extra-label' // TODO: 4 fails
'no-floating-decimal' // TODO: 155 fails
'no-labels' // TODO: 20 fails
'no-lone-blocks' // TODO: 17 fails
'no-return-await' // TODO 19 fails
'no-sequences' // TODO: 2 failures
'no-throw-literal' // TODO: 1 failure
'no-unmodified-loop-condition' // TODO: 1 failure
'no-useless-catch' // TODO: 1 fail
'no-useless-concat' // TODO: 38 fails
'no-useless-return' // TODO: 5 fails
'prefer-promise-reject-errors' // TODO: 14 failures
'require-await' // TODO: 53 fails
'require-unicode-regexp' // TODO: 272 fails
yoda // TODO: 9 fails
strict // TODO: 338 fails
'linebreak-style' // TODO 726 fails
'no-unneeded-ternary' // TODO: 5 fails
'prefer-object-spread' // TODO: Only 1 fail
If anyone wants to test-drive the above rules... According to @samreid, they are split across chipper/eslint/.eslintrc.js and chipper/eslint/sim_eslintrc.js
@zepumph recommends skipping these rules:
@samreid tested with grunt lint-everything --disable-eslint-cache
.
Moving forward, @samreid will fix any that are automatically fixable, and possibly anything that looks trivial.
After that is complete, developers can test on their own and address anything as needed, then we'll revisit in dev meeting next week.
I pushed the "no-eval" rule to perennial, and it said it will automatically fix the version in chipper (SimVersion copy).
I decided to fix the chipper version eagerly, and I left a fake comment so daily grunt work can still do a commit, in case that helps?
Another level would be to make those rules warnings for a while, but our tooling might treat warnings the same as it treats errors?
I've completed the auto and easy fixes. After the commits, here are the remaining failing rules:
I removed the TODO next to the indent
rule, as we experimented with it in our format rules, and I do not think it will make the cut. After that there are only 112 errors in RaP when I turn on all rules that look like 'off', // TODO
in the eslintrc file. I'll keep investigating.
sort-imports
will ever work for us. These two rules don't combine well https://www.jetbrains.com/help/webstorm/settings-code-style-javascript.html#ws_js_settings_editor_code_style_imports_tab https://eslint.org/docs/rules/sort-imports. ESlint (and michael) want to sort based on member name (the left side of the import), but Webstorm sorts by module name. I would recommend turning this off and removing the TODO as a no-fix sorta scenario. After turning off sort-imports
, ratio and proportion only has 2 lint errors. Wahoo!
Very close to 100% of the auto-fixes made to my sim were for the prefer-template
rule, which prefers string templates over string concatenation. I have been using templates where appropriate since that feature because available. So most of the auto-fixes are in code that pre-dated the availablity of the JS string template feature. Most are nice improvements, so I think turning this rule on (even if it was only temporarily) was beneficial.
An example of what I consider a "good fix" is https://github.com/phetsims/acid-base-solutions/commit/45c8579524c5ec5bc1a186e3926dd81822a304db:
- this.addChild( new RichText( '10<sup>' + ( i - 8 ) + '</sup>', {
+ this.addChild( new RichText( `10<sup>${i - 8}</sup>`, {
But there are some auto-fixes that are questionable -- a tiny step in the wrong direction imo. For example, when adding a prefix or suffix to an existing string, I feel that concentation is clearer, and the template syntax is less clear and unnecessarily verbose. For example https://github.com/phetsims/ph-scale/commit/961c54d3dfc23e020f519fc23721f572731a008b:
- tandemName: solute.tandemName + 'Item' // Item suffix is required by ComboBoxItem
+ tandemName: `${solute.tandemName}Item` // Item suffix is required by ComboBoxItem
Templates and concatenation are both useful, valid options. I don't like having a rule that takes one of those options away from me. If programmers don't know when to use a tool (in this case concatenation), the solution is not to take the tool away. I'm happy to go with whatever PhET decides, but I'd like to discuss, so that banning concatenation is a consensus. My vote would be to turn off the prefer-template
rule.
Regarding prefer-template
, I don't feel too strongly, but I agree the vast majority of changes from applying that auto-fix were improvements. Personally I don't see solute.tandemName + 'Item'
as a significant improvement over ${solute.tandemName}Item
, but I'm happy to go with whatever the team consensus is. Or if these cases are rare enough, we could enable the rule and require exceptions to use // eslint-disable-line prefer-template
.
There are many lint errors on CT because CT doesn't pull aqua. @jonathanolson and I pulled aqua on bayes, and things should clear up now.
Thanks! I pulled aqua on CT earlier but forgot to do it a second time. I appreciate it.
I looked over the remaining rules, and here are my votes about them:
Either way:
'array-callback-return'
'consistent-return'
'no-labels'
- I've never used them, and never will'no-return-await'
'require-unicode-regexp'
, but also kinda don't want, because why? And it isn't easy to batch fix current usages.strict
- I don't think it matters where, but I also would rather not add this if I were the one that had to fix usages.'prefer-rest-params'
- this feels like a logic change in how I code. I would be fine adopting it, but it seems less style, and more opinionated than some of the other rules.'prefer-spread'
- I don't care, whatever people want.Want:
no-async-promise-executor
'require-atomic-updates'
'default-param-last'
What about a default param right before options though, do we allow that?'no-constructor-return'
'no-lone-blocks'
- generally seems like it wouldn't pass code review.'no-sequences'
'no-throw-literal'
'no-unmodified-loop-condition'
'prefer-promise-reject-errors'
This would help me write better Node code.'require-await'
Don't want:
Briefly scanned the proposed rules, everything looked generally good.
Summary of decision and discussion from the above, hidden comments (since there are currently 268 hidden comments)
indent
rule should not be turned on, https://github.com/phetsims/chipper/issues/814#issuecomment-797667066sort-imports
does not work well because it sorts differently from webstorm https://github.com/phetsims/chipper/issues/814#issuecomment-797671749prefer-template
: We, as a team are very ambivalent about this one, @pixelzoom comments in https://github.com/phetsims/chipper/issues/814#issuecomment-797688106. Pretty much people prefer templates most of the time, but also don't love being pigeon-holed by this. We will pick up discussion next time.
SR: I don't feel strongly one way or the other about this rule, but my experience has been that I am trained reflexively to use concatenation even when it's sub-optimal. I was also encouraged that this can be fixed either with grunt lint --fix
or with the IDE with 2 keystrokes:Do we use concatenation rarely enough that intentional uses can opt out of the rule with // eslint-disable-line prefer-template
? I wonder if we will get better at just learning to use templates in the first place.
@zepumph said in his comment in with the below block https://github.com/phetsims/chipper/issues/814#issuecomment-797688106.
Either way:
'array-callback-return'
'consistent-return'
'no-labels'
- I've never used them, and never will'no-return-await'
'require-unicode-regexp'
, but also kinda don't want, because why? And it isn't easy to batch fix current usages.strict
- I don't think it matters where, but I also would rather not add this if I were the one that had to fix usages.'prefer-rest-params'
- this feels like a logic change in how I code. I would be fine adopting it, but it seems less style, and more opinionated than some of the other rules.'prefer-spread'
- I don't care, whatever people want.Want:
no-async-promise-executor
'require-atomic-updates'
'default-param-last'
What about a default param right before options though, do we allow that?'no-constructor-return'
'no-lone-blocks'
- generally seems like it wouldn't pass code review.'no-sequences'
'no-throw-literal'
'no-unmodified-loop-condition'
'prefer-promise-reject-errors'
This would help me write better Node code.'require-await'
@jonathanolson responded to this and the whole issue "Briefly scanned the proposed rules, everything looked generally good."
UPDATE: See below for a threaded comment:
This issue adds a section for each remaining rule, and can be used for threading the conversation. Please feel free to update this comment with your notes. I'll add comments from the previous rule.
MK: I could go either way on this. SR: I recommend enabling this rule. It seems easy to fix, and the existing occurrences look buggy. Dev team: Yes!
MK: I could go either way on this. SR: I recommend enabling this rule. The existing occurrences look buggy. Dev team: Yes, assuming this doesn't affect cases where no return value was expected
MK: I recommend we adopt this rule. What about a default param right before options though, do we allow that? SR: I recommend enabling this rule. The existing occurrence looks buggy. Dev team: Yes
MK: I recommend we adopt this rule. SR: I also recommend enabling this rule. Dev team: Yes
MK: I recommend we adopt this rule. SR: Me too! Dev team: Yes
MK: I could go either way on this--I've never used them, and never will. SR: I think we should forbid labels. If there are few spots that require them, they can opt out of the rule for that line. Dev team: Yes, and disable eslint by line where needed
MK: I recommend we adopt this rule. generally seems like it wouldn't pass code review. SR: This seems like it warrants discussion--I know we used this pattern more in Java. I don't really like the pattern but maybe others think it's effective? Dev team: Deferring, created a new issue to discuss further: https://github.com/phetsims/chipper/issues/1026
MK: I could go either way on this SR: It seems like we should turn this off for clarity. Dev team: Yes, because it is generally safe to remove the return from the await
MK: I recommend we adopt this rule. SR: Sequences are confusing and non-idiomatic, they should be forbidden. Dev team: Yes
MK: I recommend we adopt this rule. SR: This looks buggy, it should be fixed. Dev team: Yes
MK: I recommend we adopt this rule. SR: This looks buggy, it should be fixed. Dev team: Yes
MK: I recommend we adopt this rule. This would help me write better Node code. SR: I recommend we adopt this rule. Dev team: Yes
MK: I could go either way on this. this feels like a logic change in how I code. I would be fine adopting it, but it seems less style, and more opinionated than some of the other rules. SR: I'm not really sure, would like to look at examples and discuss with the team. Dev team: Yes, but with the option to opt out if there is a performance concern
MK: I could go either way on this. I don't care, whatever people want. SR: I'm not really sure, would like to look at examples and discuss with the team. Dev team: Yes
We, as a team are very ambivalent about this one, @pixelzoom comments in https://github.com/phetsims/chipper/issues/814#issuecomment-797688106. Pretty much people prefer templates most of the time, but also don't love being pigeon-holed by this. We will pick up discussion next time.
SR: I don't feel strongly one way or the other about this rule, but my experience has been that I am trained reflexively to use concatenation even when it's sub-optimal. I was also encouraged that this can be fixed either with grunt lint --fix
or with the IDE with 2 keystrokes:
Do we use concatenation rarely enough that intentional uses can opt out of the rule with // eslint-disable-line prefer-template
? I wonder if we will get better at just learning to use templates in the first place.
Dev team: We would like to wait for @pixelzoom and discuss in a new issue, see https://github.com/phetsims/chipper/issues/1027
MK: I recommend we adopt this rule. SR: This seems like it could help us catch potential latent bugs, we should enable the rule. Dev team: Yes, we will have to disable a few lines in Rosetta and make an issue until @jbphet and @liam-mulhall can investigate
MK: I recommend we adopt this rule. SR: This seems like it could help us catch potential latent bugs, we should enable the rule. Dev team: No, because async functions that don't use await would fail, and @jonathanolson showed examples of this being a convenient case to allow. We opened a new issue for discussion, see
MK: I could go either way on this. but also kinda don't want, because why? And it isn't easy to batch fix current usages. SR: What is the reason this rule exists? Dev team: Yes. Mildly nice to have, but autofix can't handle these, so this will be an effort to fix. New issue here: https://github.com/phetsims/chipper/issues/1029
MK: I could go either way on this. I don't think it matters where, but I also would rather not add this if I were the one that had to fix usages. SR: I'm not sure how this rule helps, but I think it has autofix, so maybe just turn it on and forget about it? Dev team: Yes, we'll use autofix
Here is the output from linting with those rules all turned on:
I volunteered to fix as many issues as I can based on the dev meeting consensuses in the above comment. For issues I cannot fix, I will create side issues or ask for help, or mark things as eslint-disable.
Here is my list of remaining rules to enable:
consistent-return https://github.com/phetsims/chipper/issues/1033 default-param-last no-async-promise-executor no-constructor-return no-labels no-return-await no-sequences no-throw-literal no-unmodified-loop-condition prefer-promise-reject-errors prefer-rest-params prefer-spread require-atomic-updates require-unicode-regexp https://github.com/phetsims/chipper/issues/1029 strict
All rules recommended by the team have been enabled or moved to side issues, closing.
Today @chrisklus @zepumph and I discussed the following possibility:
The idea is that the rules we are breaking a dozen or less times across our codebase are already closely in line with our philosophy, and there is a good chance we could benefit from adopting the rules.
@zepumph volunteered to investigate at seriously low priority.