From 922dbb2f676b1db4309208b0fb873a91f47cf89b Mon Sep 17 00:00:00 2001 From: Sam Reid Date: Tue, 30 Jul 2024 12:59:15 -0600 Subject: [PATCH] Update code review checklist regarding disposal, see https://github.com/phetsims/phet-info/issues/224 --- checklists/code-review-checklist.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/checklists/code-review-checklist.md b/checklists/code-review-checklist.md index 60ba4631..50e19dce 100644 --- a/checklists/code-review-checklist.md +++ b/checklists/code-review-checklist.md @@ -86,9 +86,10 @@ If any of these items fail, pause code review. by `ObservableArrayDef.element*Emitter.removeListener` * SCENERY: `Node.addInputListener` is accompanied by `removeInputListener` * TANDEM: Creation of an instrumented `PhetioObject` is accompanied by `dispose`. -- [ ] All classes that require a `dispose` function should have one. This should expose a public `dispose` function that - calls `this.dispose{{CLASS_NAME}}()`, where `dispose{{CLASS_NAME}}` is a private function declared in the - constructor. `{{CLASS_NAME}}` should exactly match the class name. +- [ ] All classes that require a `dispose` function should have one. There are many reasonable ways to implement +`dispose`, including but not limited to: (a) declaring a private function like `this.dispose{{CLASS_NAME}}()` in +the constructor, where `{{CLASS_NAME}}` should exactly match the class name. (b) Using Disposable.ts/disposeEmitter, +(c) calling `super.dispose()` and `myChild.dispose()` etc. - [ ] All classes that do not properly override `dispose` should either (a) use `isDisposable: false`, or (b) implement a `dispose` method that calls `Disposable.assertNotDisposable`. Use (a) for classes that inherit a `dispose` method from Disposable. Use (b) for classes that inherit a `dispose` method from something other than Disposable. The goal