phetsims / assert

Minimal standalone assertion support for PhET libraries
http://scenerystack.org/
MIT License
2 stars 3 forks source link

Lint rule should forbid string literals in assert() #7

Closed samreid closed 2 years ago

samreid commented 2 years ago

During https://github.com/phetsims/scenery/issues/1407 I noticed around 10 occurrences like:

assert && assert( 'Need an active Node to update line width' );

Note that the string 'Need an active Node to update line width' is truthy, so this is equivalent to calling assert && assert(true) with no error message. Therefore it will never trigger. This can probably be also caught through type checking, but the type signature must allow string|null (for cases like assert(this.myNullableString,'string should not be null by now) so it may not be trivial. But a bad-text lint rule would be very easy. Here are the current failures:

Running "lint-everything" task

/Users/samreid/apache-document-root/main/circuit-construction-kit-common/js/view/CircuitElementNumberControl.ts
  84:15  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/equality-explorer/js/common/view/ObjectPicker.js
  368:21  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/neuron/js/neuron/model/NeuronModel.js
   852:17  error  Line contains bad text: 'assert( ''  bad-text
  1073:19  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery-phet/js/NumberPicker.js
  532:21  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/input/SimpleDragHandler.js
  343:15  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/listeners/KeyboardDragListener.ts
  886:17  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/nodes/WebGLNode.ts
  152:15  error  Line contains bad text: 'assert( ''  bad-text

/Users/samreid/apache-document-root/main/scenery/js/overlays/HighlightOverlay.ts
  652:17  error  Line contains bad text: 'assert( ''  bad-text
  676:15  error  Line contains bad text: 'assert( ''  bad-text

✖ 10 problems (10 errors, 0 warnings)

Warning: 10 errors and 0 warnings Use --force to continue.
samreid commented 2 years ago

I proposed a fix in the commits. @jessegreenberg can you please review?

jessegreenberg commented 2 years ago

Good idea, this is a good thing to catch. I tested the lint rule locally and it works. In https://github.com/phetsims/scenery/issues/1408 this helped identify that a legitimate expression missing in one of the assertions. I reviewed the other changes to the assert usages in this issue and they seem correct.

Thanks! Ready to close?

samreid commented 2 years ago

Thanks, closing.