`bad-typescript-text` custom lint rule

[zepumph]

We would like @public, @private, and @protected to be forbidden in typescript files. I think a bad text rule just in the ts overrides section will work here.

[zepumph]

There were 1000 lint errors when I turned this rule on. Let's chip away at this!

[jessegreenberg]

What do we do for mixin/trait with this?

/* eslint-disable bad-typescript-text */

at the top of the file?

[zepumph]

I think just a line directive at every comment where we have those texts.

[pixelzoom]

Should @returns be added to this rule?

EDIT: The answer is no, because we want to be able to optionally document the return type, like in CarouselButton.ts:

/**
...
 * @returns the pointer area, null if no dilation is necessary, i.e. x === 0 && y === 0
 */
function computePointerArea( button: CarouselButton, arrowDirection: ArrowDirection, x: number, y: number ): Bounds2 | null {

EDIT: How about making @returns by itself (no additional doc) an error?

[zepumph]

Yes good plan. Commit coming soon.

[zepumph]

Alright. I liked this regex best to allow for extra doc:

/(@returns \{)|(@returns$)/

There were something like 250 lint errors. So it isn't on right now.

[chrisklus]

From 5/9 supplemental TypeScript meeting:

PSA delivered that this is a chip-away for everyone to work on.

For the lint rule preventing instanceof or typeof asserts, we think that most of the offending cases are likely redundant and leftover from JS, but looked at some good usages. Those should be able to pass by opting out of lint for that line.

@zepumph is going to make a commit to allow for disabling by line.

[zepumph]

You can now opt out of rules like // eslint-disable-line bad-typescript-text as needed. @pixelzoom mentioned multiple good reasons to allow checking instanceof or typeof assertions in TypeScript files, so we can disable that line accordingly.

[samreid]

You can now opt out of rules like // eslint-disable-line bad-typescript-text as needed.

Excellent, nice work @zepumph !

[pixelzoom]

geometric-optics and vegas are passing all bad-typescript-text. sun and scenery-phet are close, not a problem in code that I've converted. Self unassigning.

[jessegreenberg]

We are down to 143 of these errors. For those coming back to this issue the way to enable this rule is to uncomment https://github.com/phetsims/chipper/blob/f7e06eea7ddb40dde8a8ba010a236566f634ac0b/eslint/rules/bad-typescript-text.js#L25

and

https://github.com/phetsims/chipper/blob/f7e06eea7ddb40dde8a8ba010a236566f634ac0b/eslint/rules/bad-typescript-text.js#L42-L46

Rules blocking @public and @protected have since been enabled in master.

[pixelzoom]

I fixed a bunch of the @private problems in the above commits. @samreid note that the majority of these were in bending-light and circuit-construction-kit-common. See also https://github.com/phetsims/chipper/issues/1258.

[pixelzoom]

I've fixed most of the @private problems, with these exceptions:

• axon.PhetioObject.ts is a mess. Should all of the fields prefixed with '_' be private? readonly?
• wilder.Mixable - I don't know how to address @private in mixins
• axon.createObservableArray - I don't know how to address @private in mixins

@zepumph volunteered to handle these. Then @private can be turned on in bad-typescript-text.js.

[pixelzoom]

@private has been enabled in bad-typescript-text.js, so it's no longer relevant for ts-status. Tracking removal of @private from ts-status in https://github.com/phetsims/chipper/issues/1259.

[zepumph]

There are 141 usages of @returns with no extra doc afterwards, we can work on this next.

[zepumph]

I also was able to add @param { as bad text, since we shouldn't be providing type info via jsdoc. There are still two commented out rules:

• options = merge
• assert related type checking. I'm not sure if we still want this one. There are 658 errors currently. I'll ask at next dev meeting.

[pixelzoom]

After converting a few merges in the above commits... I'm seeing mostly conversions of merge to optionize that have been "deferred", no real legitimate need to use merge. So I think we should definitely address this by adding merge to bad-typescript-text.

Here's the status of merge:

• [ ] axon @samreid - 2 errors
• [ ] bamboo @samreid - 16 errors
• [ ] bending-light @samreid - 3 errors
• [ ] circuit-construction-kit-common @samreid - 11 errors
• [ ] gravity-and-orbits @samreid - 14 errors
• [ ] greenhouse-effect @jbphet - 22 errors
• [x] scenery-phet @jonathanolson - 7 error in NumberControl, PhetFont
• [x] scenery @jonathanolson - 11 errors
• [x] number-play @chrisklus - 6 errors
• [ ] tandem @zepumph - 7 errors

[chrisklus]

From 6/16/22 dev meeting:

We checked in on this issue.

[zepumph]

Today we decided that we want to turn on the useless assert instanceof/typeof check for all repos but scenery. Here is the current chip-away for it. Basically 2/3rds are in scenery, and javascript files will use this code for long enough that it isn't worth removing these assertions. This will involve moving this particular rule out of bad-typescript-text so that scenery can opt out of it.

• [ ] circuit-construction-kit-common: @samreid 16 errors in 12 files.
• [ ] axon: @samreid 14 errors in 8 files.
• [ ] bamboo: @samreid 9 errors in 3 files.
• [ ] density-buoyancy-common: @jonathanolson 11 errors in 3 files.
• [ ] dot: @jonathanolson 17 errors in 4 files.
• [ ] joist: @zepumph 1 errors in 1 files.
• [ ] kite: @jonathanolson 101 errors in 11 files.
• [ ] mobius: @jonathanolson 2 errors in 1 files.
• [ ] models-of-the-hydrogen-atom: @pixelzoom 1 errors in 1 files.
• [ ] phet-core: @jonathanolson 3 errors in 3 files.
• [ ] scenery-phet: @pixelzoom 1 errors in 1 files.
• [ ] ~scenery: @jonathanolson 445 errors in 62 files.~
• [ ] sun: @jbphet 19 errors in 9 files.
• [ ] tambo: @jbphet 2 errors in 1 files.
• [ ] tandem: @zepumph 22 errors in 7 files.
• [ ] utterance-queue: @jessegreenberg 2 errors in 2 files.
• [ ] wilder: @jonathanolson 1 errors in 1 files.

[zepumph]

I felt the same way about kite and dot as I did for scenery, I also made the regex more specific, only testing an exact, single type check. Commit coming soon.

[zepumph]

Alright, no-simple-type-checking-assertions is now on, and a separate rule in which kite, dot, and scenery opt out. I ended up putting ~10 eslint-disable-lines in place because they seemed good and important.

[zepumph]

All that is left for this issue is to get merge out of our options in typescript.

We are down to 69 merge issues:

✖ 54 problems (54 errors, 0 warnings)

• [ ] axon: @samreid 3 errors in 2 files.
• [x] bamboo: @samreid 1 errors in 1 files.
• [ ] bending-light: @samreid 3 errors in 3 files.
• [ ] center-and-variability: @samreid, @chrisklus 1 errors in 1 files.
• [ ] circuit-construction-kit-common: @samreid 14 errors in 14 files.
• [ ] gravity-and-orbits: @samreid 11 errors in 10 files.
• [x] counting-common: @chrisklus 1 errors in 1 files.
• [ ] density-buoyancy-common: @jonathanolson 1 errors in 1 files.
• [ ] greenhouse-effect: @jbphet 15 errors in 14 files.
• [x] mobius: @jonathanolson 1 errors in 1 files.
• [x] quadrilateral: @jessegreenberg 1 errors in 1 files.
• [ ] sun: @jbphet 2 errors in 2 files. Warning: 54 errors and 0 warnings Use --force to continue.

[zepumph]

I updated the comment above, just 54 conversions left.

[zepumph]

I assigned the remaining repos to responsible devs

[pixelzoom]

In the above commits, I replaced so more occurrences of merge in sun. The remaining cases are more involved.

[samreid]

Here's a chip-away report of the remaining parts:

• [x] axon: @samreid 1 errors in ObservableArray
• [x] axon: @jonathanolson 2 errors in 1 files - UnitConversionProperty (@samreid tried this and wasn't sure how to proceed, so assigned to file author).
• [x] bending-light: @samreid 3 errors in 3 files.
• [x] center-and-variability: @samreid, @chrisklus 1 errors in 1 files.
• [x] circuit-construction-kit-common: @samreid 14 errors in 14 files.
• [x] density-buoyancy-common: @jonathanolson 1 errors in 1 files.
• [x] gravity-and-orbits: @samreid 11 errors in 10 files.
• [ ] greenhouse-effect: @jbphet 14 errors in 13 files.
• [ ] sun: @jbphet 1 errors in 1 files. ===> @pixelzoom volunteered, thanks!

[jonathanolson]

Handled all merges in density-buoyancy-common.

[samreid]

Updated chip-away result:

Results from chipAway:

• [x] joist: @zepumph 1 errors in 1 files. ===> @samreid is working in this area
• [x] scenery: @jonathanolson 1 errors in 1 files. Use combineOptions

[samreid]

We have good progress on this issue. Everything seems complete except:

• [ ] I noticed 3 files in aqua that are shutting off this rule. Would be good to visit/address those. Should it be part of this sprint? UPDATE: In today's meeting, we agreed that we don't need to address these for the sprint.
• [ ] Also, I had to re-enable the rule from usages in greenhouse-effect. (Bad cache may have prevented showing these problems).

[zepumph]

@pixelzoom or @jbphet, I went for the ButtonModel usages, can you please review my commit https://github.com/phetsims/sun/commit/0d543c76d7256e1cfc3694ff45c1be3fe9716f93

[pixelzoom]

https://github.com/phetsims/sun/commit/0d543c76d7256e1cfc3694ff45c1be3fe9716f93 looks good to me.

[pixelzoom]

Back to @zepumph because I'm not sure where this issue is at.

[zepumph]

All that is left is ~20 usages of merge in greenhouse.

[jbphet]

The greenhouse-effect usages have now all been addressed, and I've turned the rule on. I think we can close this!