myano / jenni

jenni was a python IRC bot. Project is closed. Try Sopel instead, https://sopel.chat/
https://sopel.chat/
Other
235 stars 101 forks source link

[RFC] Fix setattr on wrapped class #182

Closed solidgoldbomb closed 9 years ago

solidgoldbomb commented 9 years ago

This is a request for comments since this could cause explicit breakage on various modules where the bahaviour was formerly allowed, but silently had no effect.

Some modules are setting/updating attributes expecting them to be globally visible but because the modules only see a wrapped version of the 'jenni' object, all of these updates are lost immediately after the call to the module's method.

See: usage of is_authenticated and is_connected in startup.py and sasl.py

This is directly visible by watching the log when SASL auth succeeds and noting that startup.py still initiates the IDENTIFY with nickserv. This is due to startup.py seeing is_authenticated as False rather than the (lost in wrapped version) True set by sasl.py.

Specifically, the 903 handling in sasl.py sets jenni.is_authenticated = True. That cannot ever be seen outside of this call since it is only set in the wrapped class, not the "real" jenni instance.

This patch set allows these attributes to be set on the top-level jenni instance. It also dumps a traceback to the console for all other uses of setattr on the wrapped class since they will not have the expected effect since they vanish immediately.

kaneda commented 9 years ago

@solidgoldbomb can you highlight what other methods call this that would break as a result?

solidgoldbomb commented 9 years ago

Out-of-tree modules may be affected but no way to tell. As for in-tree, I haven't tested these but it looks like:

I can do some testing on a few of these modules with the patch applied to see if they break. Might be a couple days before I can get to that though.

Could you comment on whether the wrapper object was intended to hide/restrict these variables or if the goal was just to override the say/write methods and let everything else pass through? I see many uses throughout the code of jenni.bot.foo = bar which seem like they're compensating for the (unexpected?) setattr behaviour. If all of that jenni.bot usage is just working around a bug then I can just make the wrapper object look transparent to setattr and drop all of the workaround behaviour.

solidgoldbomb commented 9 years ago

I'm going to abandon this and open a new PR that fixes this a different way. I'll make jenni.foo = bar apply to the wrapped class (ie. the top-level jenni instance). That will be a less disruptive change.