Store AutoUnload instances instead of going through modules?

Discuss API design here.
User avatar
Mahi
Senior Member
Posts: 236
Joined: Wed Aug 29, 2012 8:39 pm
Location: Finland

Store AutoUnload instances instead of going through modules?

Postby Mahi » Wed Nov 25, 2015 11:06 pm

Due to a recent bug I had I started thinking about why AutoUnload works the way it does. Wouldn't it be much better to store all the instances of AutoUnload classes into lists or dicts, and then loop through the iterable when the plugin is unloaded, instead of going through the dir() of the modules?
User avatar
satoon101
Project Leader
Posts: 2697
Joined: Sat Jul 07, 2012 1:59 am

Postby satoon101 » Thu Nov 26, 2015 4:06 am

To me, you are trying to fix a problem of your own creation. What if I wanted to write a plugin that directly interacts with your plugin and call your _on_player_spawn function and I don't notice you redefine it later in your plugin?

In my opinion, it would be better for you to do one of 3 things. First, which you have done, is rename the function in question so you are not redefining a name in your module. Second is to utilize one function to do both functionalities instead of registering 2 functions to the same event in the same module. Third, since the function deals with a functionality in your database module, would be to register the event to a function in the database module itself.
Image
User avatar
Mahi
Senior Member
Posts: 236
Joined: Wed Aug 29, 2012 8:39 pm
Location: Finland

Postby Mahi » Thu Nov 26, 2015 12:28 pm

Yeah I know I made a mistake in my program, I'm not even remotely trying to blame this on you guys. I don't think I want to merge the two functions (is there any upside of doing so?) and I will change the database module quite a lot in the near future, so I'll keep your third option in mind when I do so.

However, this thread is not meant to be about my bug (even if the bug made me ask this), but in general about a better AutoUnload. Is there any reason to do it the way it currently works, instead of just storing all AutoUnload instances? If we were to store the intsances, it would avoid many bugs and would allow us to define AutoUnload instances inside of classes or functions, and they wouldn't have to be at the module namespace. It seems like overall better option to me, not because it would've fixed my exact bug! :)
User avatar
satoon101
Project Leader
Posts: 2697
Joined: Sat Jul 07, 2012 1:59 am

Postby satoon101 » Thu Nov 26, 2015 1:14 pm

It is a good idea and would fix another bug I have been trying to find a solution for when it comes to the PublicConVar class. I haven't been at a computer since Saturday and probably won't be until Monday. I will work on this when I do get back if no one else has by then.
Image
User avatar
satoon101
Project Leader
Posts: 2697
Joined: Sat Jul 07, 2012 1:59 am

Postby satoon101 » Fri Nov 27, 2015 3:00 pm

I ended up having a small window of time this morning to work on this.
https://github.com/Source-Python-Dev-Team/Source.Python/commit/7984c06b7ad5310033b67cbf83e181f90240a3b8
Image

Return to “API Design”

Who is online

Users browsing this forum: No registered users and 8 guests