Open rakiru opened 9 years ago
Commands: #49, #56
Rethink overzealous use of singletons.
I would be the first person to say that people complaining that Singletons are always an awful idea are stupid, but we do use a crapton of them and they aren't needed nearly as much as they used to be.
Rethink Protocol reuse (same Protocol instance reused for every connection).
We.. don't do this. Do we? I didn't think we did.
Rethink translation system
The main problem I had was that we already had 213987328746238732462 strings in Ultros and going through and replacing them with tokens manually is an extreme pain in the ass. Don't get me wrong - the system works - but I'd rather a Java-style properties approach than this stupid .po
.mo
crap, and some translation site like Crowdin.
Rethink versioning
Yeah, I'd be doing all this if I remembered to, but.. I don't. Halp!
All the things you quoted were things that I meant for separate tickets and just put here so I didn't forget. :V Does that mean you agree with all the checkboxed items? The last one should probably also be a separate ticket, but I stumbled across it while documenting the basic structure for CODING.md.
[singletons snip]
Off the top of my head, I can't think of a place where a singleton is needed. Perhaps the factory manager, but even then, you could arguably use multiple of those with different base config paths or something. They're restricting a lot of things to only one instance when there's really no need to do so, and the only reason it's been done that way is we haven't had any form of central "bot" to store references to the instances in, just a bunch of code that's been loaded and knows the rest exists by path. It also makes it impossible to switch any of this out after startup.
As for the "people complaining that Singletons are always an awful idea are stupid" argument, you could say that about anyone who sees anything in absolute black and white terms. The general reasoning behind it though is that they're so often used badly or when there's a better way to do it.
We.. don't do this. Do we? I didn't think we did.
system.factory.Factory().buildProtocol()
[translation snip]
I have no experience with translation, so I have no idea what approaches there are to it and what benefits/problems they have. I just know we've discussed the existing system we have and it doesn't really make sense for us. A separate ticket should be made for this with a list of requirements, perhaps?
Yeah, I'd be doing all this if I remembered to, but.. I don't. Halp!
Yeah, it's not something I ever remember either. Perhaps once we've discussed the issues in this ticket and got a more solid plan of what to do going forward, we can actually use the milestones feature and be a bit more liberal with the version bumps.
Does that mean you agree with all the checkboxed items?
Yis.
[Singletons]
Yeah, I agree with you, I picked singletons for ease of use to start with but moving them out is definitely not a bad idea.
system.factory.Factory().buildProtocol()
Yeah, but.. Factory() is a new instance?
[Translations]
Yep. Python uses .po
and .mo
files, which is the GNU way of doing things, but I just can't get behind it, it's too complicated.
[Versioning]
Yeah, I think so. The problem is remembering to do it - maybe we should add a git hook on commit to remind us with a popup or something.
Yis.
Awesome, I'll get on that soon.
Yeah, but.. Factory() is a new instance?
Ah, I see the confusion. Two different IRC connections will obviously get their own Protocol instance, but if IRC instance 1 dies, instead of that factory making a new IRC instance to reconnect, it reuses the same one.
Yeah, I think so. The problem is remembering to do it - maybe we should add a git hook on commit to remind us with a popup or something.
I think if we just actually have a process for adding new features (issue to plan it, separate branch, PR), then versioning can just become a part of that. A git hook that goes "Hey, you've touched one of the core files but not versions.py - does it need bumped?" could be useful, but I think it will be wrong the vast majority of times, meaning it just gets ignored.
...instead of that factory making a new IRC instance to reconnect, it reuses the same one
Ah, I see. Well, I guess we could make a new one, yeah. Shouldn't be too hard.
[Versioning]
Yeah, makes sense. We'll have to come up with some standards and stick to them - Maybe decide on not ever pushing to master unless reviewed PR or minor fix?
Shouldn't be too hard.
I'm pretty certain that's the default behaviour (and largely why I suggested it).
[Versioning]
Yeah, I was thinking something along those lines. I'm working on conventions branch again, so I'll make some provisional guidelines and we can discuss from there. I'll open a PR on it when the basics are done.
Sounds good - Will keep an eye out as I recover from this blasted viral infection
Additional stuff:
__main__.py
run.py
importableRename event_manager to events
We already have a module named events
- perhaps use events.manager
?
We have a function decorator for deprecation, so we should make a class one and wrap this.
I'm not marking this as done yet because the traceback inspection does not work right if you have both a class decorator and function decorators on that class.
I'm not sure that's an actual problem though, do we need it?
Also, we should think up how to handle deprecation (when do we actually remove things?).
When we've updated contrib and base to no longer use the deprecated things, plus.. some amoumt of time?
Rethink translation system
Note to self, @rakiru suggested Babel and it looks much better. But I still think actual translations may be better with properties files as we can upload those to sites like Crowdin for community translation.
Babel doesn't actually handle translations, just everything else (number formatting, etc.), although it does have a few utility things around gettext, which is what we're using from translations (and seems to be the best/only choice).
Discuss system.help
- Reasoning: What even is this? We should probably discuss commands a bit to figure out how they would provide help topics and such.
system.help
was my first attempt at a central help system. I was trying to keep it fairly modular, with different topic types (EG, command topics incl. syntax, and Arguments objects), but it got complicated fast and I couldn't think of a good way to deal with sub-topics. It did work, but.. not ideal.
OK, plugin cleanup is done as far as I can see, feel free to review
@rakiru Pretty much ready for review, although I'm sure there's more we could do with that Ultros class. I'm sure you'll find a bunch of things to change anyway :P
So.. We should discuss the help system next I guess
Eh, late Summer clean? Or more likely, Autumn to Winter clean. There's a lot of crap in
utils
that just got shoved there at the beginning without any real thought behind it. It may be worth having a scan through the rest of the code for less-than-stellar code (instance variables set to mutable type in class definition, for example (or even just instance variables referenced in class defs at all)). Perhaps Landscape might be useful for once.Edit: Some possible changes too:
command_manager
tocommands
command_manager.CommandManager
is a bit redundant, and any other command-related stuff would probably go in there too. This could also be a full module rather than a single file.event_manager
toevents
commands
system.plugin
Reasoning: It's just an import ofsystem.plugins.plugin.PluginObject
. We have a function decorator for deprecation, so we should make a class one and wrap this.Manager
infactory_manager
toFactoryManager
run.py
into anUltros
class.system.help
Any renaming should be deprecated. Also, we should think up how to handle deprecation (when do we actually remove things?).
Other things to consider but should be discussed in separate tickets if/when tackled:
Rethink Protocol reuse (same Protocol instance reused for every connection).