Open S-S-X opened 3 years ago
Good catch. Would you mind adding a test? (possibly also one for a stub
if that makes sense)
I'm having second thoughts about this. This might have side effects, the table is protected for a reason, just overriding that behaviour is dangerous.
I think possible side-effects for this aren't worth the fix for this particular case. Maybe we should skip this one...
wdyt @DorianGray ?
I'm having second thoughts about this. This might have side effects, the table is protected for a reason, just overriding that behaviour is dangerous.
You're kind of right but thing is original behavior is also dangerous because normally you wont expect meta methods to be called when you're hooking debugger into your code.
For debugger (like what spy basically is) all possible side effects should be avoided, yes it would be nice if one could track calls and modifications without altering original object but that would be way more complicated change.
For simple example __newindex could be used to implement entry counter for you class, if debugger causes counter incrementing then your program behavior will be different with debugger. This has nasty side effects like off by one errors passing tests when spy is used but program actually failing when you're actually running it without tests.
So basically why I think it would be more correct to use rawset
is simply that tests should not alter program behavior if it is anyhow possible to avoid altering behavior.
But then again for same reason explained in previous message:
This might affect some use cases where it is expected that __newindex is called
With that I basically mean that some might have written tests and tested program in a way that program is actually aware of tests and expects tests to alter behavior, while this is wrong approach it still should be take into account because altering that behavior can cause some tests that succeed before to fail afterwards.
Allows using spy with tables that error on __newindex.
For example:
Would fail on
spy.on(myinstance, "foo")
because of errorNo new indexes for you!
. Withrawset
it succeeds because__newindex
is never called.Executing test script
Example output without changes from this PR:
With changes from this PR:
What and why it should use rawset
This might affect some use cases where it is expected that
__newindex
is called, any ideas and is this acceptable approach? However I think adding spy should not be visible to target or call any metamethods of target, taking this into account I think this would be most correct quick and easy fix.