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

Trove should support more object cleanup methods #149

Merged
merged 11 commits into from Nov 25, 2023
Merged

Trove should support more object cleanup methods #149

merged 11 commits into from Nov 25, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 14, 2023

Right now, Trove only supports Destroy and Disconnect as object cleanup methods. Cleanup methods other than these 2 have to be specified through Trove:Add.

However, Trove should also support camel case versions of these cleanup methods, as developers tend to follow various style guides when writing code. However, the Roblox Lua Style Guide generally states that methods should be generally camelCase.

For those who follow this (widely-used) style guide, it is inconvenient for them to specify the cleanup method through Trove:Add..

This PR makes Trove support the following *additional generic object cleanup methods:

  • destroy
  • disconnect

bubshayz and others added 2 commits August 14, 2023 19:26
Right now, Trove only supports `Destroy` and `Disconnect` as object cleanup methods. Cleanup methods other than these 2 have to be specified through `Trove:Add`.

However, Trove *should* also support camel case versions of these cleanup methods, as developers tend to follow various style guides when writing code. However, the [Roblox Lua Style Guide](https://roblox.github.io/lua-style-guide/) generally states that methods should be generally `camelCase`.

For those who follow this (widely-used) style guide, it is inconvenient for them to specify the cleanup method through `Trove:Add`..

 This PR makes Trove support the following generic object cleanup methods (which are widely used):

- `Destroy`
- `Disconnect`
- `Cleanup`
- `destroy`
- `disconnect`
- `cleanup`
@Sleitnick Sleitnick added the enhancement New feature or request label Aug 31, 2023
@Sleitnick
Copy link
Owner

Overall change looks good, thanks! Please make the changes to satisfy the lint and styling rules, then I'll merge it in.

@ghost
Copy link
Author

ghost commented Sep 1, 2023

More cleanup methods are supported (Clean, Deinit, clean, deinit)

@Sleitnick
Copy link
Owner

I'm not sure that Clean and Deinit should be included. It becomes a bit of a slippery slope of what should/shouldn't be added to such a list, e.g. Deconstruct, Remove, Drop, Close, etc. The Add method already lets you give it a custom cleanup method. I think I'm fine with adding camelCase versions of the existing ones, but not sure adding others outside of those makes much sense in the long-run.

@ghost
Copy link
Author

ghost commented Nov 7, 2023

I'm not sure that Clean and Deinit should be included. It becomes a bit of a slippery slope of what should/shouldn't be added to such a list, e.g. Deconstruct, Remove, Drop, Close, etc. The Add method already lets you give it a custom cleanup method. I think I'm fine with adding camelCase versions of the existing ones, but not sure adding others outside of those makes much sense in the long-run.

Rectified.

@Sleitnick Sleitnick merged commit 57e58d1 into Sleitnick:main Nov 25, 2023
2 checks passed
@Sleitnick
Copy link
Owner

Pushed, Trove v1.1.0. Thanks @bubshayz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant