-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add ::MSU.new function #257
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
base: development
Are you sure you want to change the base?
Conversation
|
I don't like the function part of this, I understand why it's convenient but imo it's not as 'proper' to shorthand that part of this. I'm fine with modifying fields in a constructor style function though I guess. |
b825c3c to
a75ea82
Compare
| return typeof _object == "table" && "_release_hook_DO_NOT_delete_it_" in _object; | ||
| } | ||
|
|
||
| ::MSU.new <- function( _script, _function = null ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can function be null? this seems antithetical to the purpose of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought to allow it to be null was so that ::MSU.new can be used in place of vanilla ::new without causing any issues. E.g. like we do with ::MSU.isKindOf as a replacement for vanilla ::isKindOf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, I don't think that's necessary and I don't think we should handle that, if someone really needs a null equivalent for some unknown reason they can pass an empty function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is that if someone went ahead and did find&replace on this.isKindOf in vanilla codebase to ::MSU.isKindOf and similarly on x == y to ::MSU.isEqual(x, y) it will not break the code. But if someone replaced this.new with ::MSU.new it will break if _function is required parameter. Basically just like our new global functions make it so that you never have to use vanilla this.isKindOf and similarly never have to do == when comparing instances of BB classes, we now say that you can safely always use ::MSU.new instead of ::new. That's the only reason - so if you think that reason is not valid enough then I can remove the optional param and make it required.
6677ac8 to
ae825b7
Compare
This allows creating objects with modified fields in-line. Use cases:
Modifying values
This is an extreme example. While hooking weapons for Reforged, I very frequently ran into the situation where I wanted to change one or two values of a skill. In that case this is very convenient:
Initializing values
As
create()does not take arguments, I've often worked around it by creating aninitfunction in my classes to set initialization values. This new method provides an alternative way to achieve that, reducing the need for an extra function:Thoughts on
_fieldsparameterEDIT: On second thought, it is probably ok to remove this parameter and only keep
_scriptand_function.In theory the_fieldsparameter is unnecessary as_functioncan accomplish changing fields too. But since setting fields is the most common thing to change during modifying and/or initializing an object, it is much more convenient to just pass a table instead of writing a function every time. For example:Therefore, I think it makes sense to keep both parameters.