reload: Recurse into packaged modules

[dgw]

Force-reload all sub-modules of packages recursively.

I dare say this actually fixes (most of) #1056 after two years. My testing on Python 3.6 has been promising so far, and I've heard good things from @ralienpp in that thread about its effects under Python 2.7.

My fingers remain crossed. But someone else should definitely look at this before it gets merged to make sure I didn't do something dumb. I played with the logic a lot yesterday, and after all those little tweaks I'm kind of fuzzy on what I actually did. 🤣

(Also, note to self: Find that StackOverflow post again and amend the commit to reference it in a code comment somewhere before merging.)

[kwaaak]

Thanks, I will have a look soon. This should make working on those packaged modules a lot easier.

[kwaaak]

Every @sopel.module.rule needs to have an associated @sopel.module.priority decorator or the reload will fail:

Signature: AttributeError: 'function' object has no attribute 'priority' (file "/home/sopel/sopel/sopel/bot.py", line 216, in unregister)
from nickname at 2018-05-01 15:21:44.901021. Message was: -reload youtube
Traceback (most recent call last):
  File "/home/sopel/sopel/sopel/bot.py", line 475, in call
    exit_code = func(sopel, trigger)
  File "/home/sopel/sopel/sopel/modules/reload.py", line 155, in pm_f_reload
    f_reload(bot, trigger)
  File "/home/sopel/sopel/sopel/modules/reload.py", line 51, in f_reload
    reload_module_tree(bot, name)
  File "/home/sopel/sopel/sopel/modules/reload.py", line 75, in reload_module_tree
    reload_module_tree(bot, obj.__name__, seen)
  File "/home/sopel/sopel/sopel/modules/reload.py", line 67, in reload_module_tree
    bot.unregister(obj)
  File "/home/sopel/sopel/sopel/bot.py", line 216, in unregister
    callb_list = self._callables[obj.priority][rule]
AttributeError: 'function' object has no attribute 'priority'

Once that is fixed, there are still problems with reloading the https://github.com/sopel-irc/sopel-youtube module which locks up the bot.

[dgw]

@kwaaak Is the problem with reloading sopel-youtube specific to this PR (caused by the changes)? If not, it belongs in a separate issue.

Is there no error associated with loading the module as well?

sopel/sopel/bot.py

Lines 232 to 233 in 253fd9b

	for rule in callbl.rule:
	self._callables[callbl.priority][rule].append(callbl)

would seem to require a priority to be set while registering the rule too.

[dgw]

@calebj Have you given this a look at all? It seems there's a fair bit of overlap between this and your #1053, so we should probably work together on these. I'll check for the issues this is supposed to fix when I full-on test your changes, and I guess we can go from there.

[dgw]

Follow-on to #1056 (comment) but keeping it here as it relates to this PR directly:

As previously tested, updated methods work great. Adding new methods works as expected, too.

Removing methods, though, causes issues. Sopel will repeat a removed command (1 + number_of_module_reloads_since_removal) times, instead of removing it.

At present, that's still better than #1053, which added 4 repetitions of the removed test command after a single reload. But neither solution is perfect, yet. I have some poking around to do, once more.

(This might get merged with the removed-method problem unfixed if I can't find it shortly. Fixing that isn't worth holding up the rest of the patch. Can't let perfect be the enemy of good, especially since fixing the issue with removed methods is a diminishing-returns prospect: high complexity, low incidence.)

[RustyBower]

13:45:04 <RustyCloud> .weather 90210
13:45:05 <sopel> Beverly Hills, US: 15°C (59°F), Rain, Humidity: 93%, Light breeze 3.1m/s (←)
13:45:07 <RustyCloud> .reload
13:45:08 <sopel> RustyCloud: done
13:45:09 <RustyCloud> .weather 90210
13:45:10 <sopel> Beverly Hills, US: 15°C (59°F), Rain, Humidity: 93%, Light breeze 3.1m/s (←)
13:45:11 Beverly Hills, US: 15°C (59°F), Rain, Humidity: 93%, Light breeze 3.1m/s (←)
13:45:14 <RustyCloud> .reload
13:45:15 <sopel> RustyCloud: done
13:45:16 <RustyCloud> .weather 90210
13:45:16 <sopel> Beverly Hills, US: 15°C (59°F), Rain, Humidity: 93%, Light breeze 3.1m/s (←)
13:45:18 Beverly Hills, US: 15°C (59°F), Rain, Humidity: 93%, Light breeze 3.1m/s (←)
13:45:19 ...

This is pulling in #1314 via git fetch origin pull/1314/head:reload_module and git checkout reload_module

[dgw]

@RustyBower I was really confused by that log until I tested again myself. The non-standard W|A output is a remnant of the last time I played with this, I think. This is after checking out a new test-1314 branch with git checkout -b test-1314 master and then git merge pr-1314 (which I created using a similar git fetch command to the one you used):

<~dgw>  .wa test
<Sopel> [W|A] Wolfram|Alpha API app ID not configured. Baka.
<~dgw>  Sopel: reload wolfram
<Sopel> dgw: <module 'wolfram' from '/home/dgw/.sopel/modules/wolfram/__init__.py'>
        (version: 2018-08-27 04:40:13)
<~dgw>  .wa test
<Sopel> [W|A] Wolfram|Alpha API app ID not configured. Baka.
<~dgw>  Sopel: reload
<Sopel> dgw: done
<~dgw>  .wa test
<Sopel> [W|A] Wolfram|Alpha API app ID not configured. Baka.
<Sopel> [W|A] Wolfram|Alpha API app ID not configured. Baka.
<~dgw>  Sopel: reload wolfram
<Sopel> dgw: <module 'wolfram' from '/home/dgw/.sopel/modules/wolfram/__init__.py'>
        (version: 2018-08-27 04:40:13)
<~dgw>  .wa test
<Sopel> [W|A] Wolfram|Alpha API app ID not configured. Baka.

You can see where I realized that I had only tested reloading a specific module. 😸

So, it looks like this PR is an improvement, as I thought, but fails to handle the "reload all" case. (The existing code calling bot.setup() for that case also seems to break some shutdown stuff, as evidenced by errors in my terminal after hitting Ctrl+C…)

I'm going to play with it some more, because if I can fix that in the next day or two I would love to pull this back into 6.6.0. Got an idea to start with, so here goes nothing.

[dgw]

Update: My idea worked! I half expected it to break something, but it seems to work fine.

@RustyBower could you test the update? (And @HumorBaby, since I see your comments in the other thread? :P)

[HumorBaby]

Did some nefarious testing as mentioned in #sopel. Works as expected 😄