Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Noncomplicance with CRC requirements for dispose #202

Closed
pixelzoom opened this issue Feb 27, 2024 · 4 comments
Closed

Noncomplicance with CRC requirements for dispose #202

pixelzoom opened this issue Feb 27, 2024 · 4 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 27, 2024

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
Copy link
Contributor Author

pixelzoom commented Feb 27, 2024

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

Yes, I see this in package.json:

              "forbiddenTextObjects": [
                "dispose"
              ]

@samreid samreid added the dev:help-wanted Extra attention is needed label Feb 27, 2024
@samreid samreid removed their assignment Feb 27, 2024
@samreid
Copy link
Member

samreid commented Mar 1, 2024

@matthew-blackman let's discuss this

@pixelzoom
Copy link
Contributor Author

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.

@matthew-blackman matthew-blackman removed the dev:help-wanted Extra attention is needed label Mar 8, 2024
samreid added a commit that referenced this issue Mar 8, 2024
@samreid
Copy link
Member

samreid commented Mar 8, 2024

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

@samreid samreid closed this as completed Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants