phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Noncomplicance with CRC requirements for dispose #202

Closed pixelzoom closed 5 months ago

pixelzoom commented 5 months ago

For code review #32 ...

implementation-notes.md says:

  • Objects are not dynamically allocated, and are instead statically allocated.

... so presumably nothing needs to be disposed, and I can confirm that I see no calls to dispose.

But this sim is technically not in complicance with this CRC item:

  • [ ] All classes that do not properly override dispose should either (a) use isDisposable: false, or (b) implement a dispose method that calls Disposable.assertNotDisposable.

I see only 3 occurrences of (a) and zero occurrences of (b) in the entire code base.

Imo, this CRC item is essential in common code or in a sim that disposes some things, but not others. It's less important in a sim like this that disposes of nothing -- unless it might use dispose in the future? So... Up to you whether to do anything here.

pixelzoom commented 5 months ago

... unless it might use dispose in the future?

Yes, I see this in package.json:

              "forbiddenTextObjects": [
                "dispose"
              ]
samreid commented 5 months ago

@matthew-blackman let's discuss this

pixelzoom commented 5 months ago

It would probably be OK if you noted this non-complicance in implementation-notes.md, along with how/why you're using "forbiddenTextObjects". I don't see a lot of value in retroactively adding isDisposable: false.

samreid commented 5 months ago

Thanks, @matthew-blackman and I agreed to document this as prescribed above. Closing.