topkecleon / otouto

A Lua-based Telegram bot with plugins.
http://otou.to
GNU Affero General Public License v3.0
173 stars 112 forks source link

[RFC] Less global magic #48

Closed bb010g closed 8 years ago

bb010g commented 8 years ago

This will make every previous global variable local and properly required. Currently in progress, but administration and the other changed plugins should give you a good feel for how it it is in use in the wild. Also fixes a bunch of triggers, makes a future switch to regex easier, and fixes some leakage of HTTP global state from lastfm.


This change is Reviewable

topkecleon commented 8 years ago

Does it run?

bb010g commented 8 years ago

Ish. I'm working on finishing up the rest of the plugins (couldn't really work on it yesterday due to computer hardware issues), but the first couple should work, and the core should work. I was mainly putting this out here now so you could give feedback if you wanted.

bb010g commented 8 years ago

Ok. This breaks pretty much every plugin not in otouto proper, but the refactoring's done as far as I know right now. Test at your own risk.

bb010g commented 8 years ago

Review status: 0 of 54 files reviewed at latest revision, 6 unresolved discussions.


_bindings.lua, line 11 [r2] (raw file):_ Right now this is gone and the bot will just fail saying that it can't find self.BASE_URL. Do you want this back in bot.lua when it starts up, or are you fine with it being gone?


utilities.lua, line 277 [r3] (raw file): This needs a better name, but I couldn't think of one. If you have any suggestions, please give them.


plugins/about.lua, line 18 [r1] (raw file): Done.


plugins/administration.lua, line 1135 [r3] (raw file): self.admin_temp


plugins/cats.lua, line 17 [r3] (raw file): triggers(self.info.username), move to init


plugins/gImages.lua, line 2 [r3] (raw file): Spelling


Comments from Reviewable

bb010g commented 8 years ago

Review status: 0 of 54 files reviewed at latest revision, 4 unresolved discussions.


_bindings.lua, line 11 [r2] (raw file):_ Nevermind, this was already moved! Good job, past me.


Comments from Reviewable

bb010g commented 8 years ago

Reviewed 4 of 41 files at r1, 36 of 49 files at r2, 2 of 4 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, 2 of 2 files at r11. Review status: 49 of 54 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

topkecleon commented 8 years ago

Sorry to break your automatic merging potential. :)

bb010g commented 8 years ago

Reviewed 2 of 6 files at r12. Review status: 49 of 54 files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

bb010g commented 8 years ago

Reviewed 2 of 6 files at r12. Review status: 51 of 54 files reviewed at latest revision, 6 unresolved discussions.


bot.lua, line 30 [r12] (raw file): Should be plugins.whatever for loading consistency. This goes along with removing the .luas from plugins.lua.


plugins/administration.lua, line 327 [r12] (raw file): self


plugins/administration.lua, line 375 [r12] (raw file): self


plugins/administration.lua, line 802 [r12] (raw file): self


plugins/administration.lua, line 990 [r12] (raw file): self


Comments from Reviewable

bb010g commented 8 years ago

Reviewed 1 of 49 files at r2, 5 of 5 files at r13, 1 of 1 files at r14. Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

bb010g commented 8 years ago

Ok. With that, the comprehensive review (thanks to @TiagoDanin for catching a missing return utilities) done, this should be ready to merge after some testing just to make sure. Only thing I don't quite like is utilities.INVOCATION_PATTERN, but I can't come up with a better name. Anyhow, that can be solved later, and it's mostly unnoticed thanks to utilities.triggers().

topkecleon commented 8 years ago

Will commence private testing.

christoga commented 8 years ago

Fix the git conflict dude