randy3k / AutomaticPackageReloader

Automatically reload submodules while developing a Sublime Text package.
MIT License
38 stars 13 forks source link

Call plugin_loaded on target modules #19

Open Varriount opened 6 years ago

Varriount commented 6 years ago

From my investigations into https://github.com/Varriount/NimLime/pull/121 , I've found that AutomaticPackageReloader doesn't appear to call the plugin_loaded function on target modules. This function needs to be called, as several plugins perform necessary startup actions there.

randy3k commented 6 years ago

This line should execuate the plugin_loaded function, afaik.

Varriount commented 6 years ago

The reload_plugin function only calls plugin_loaded on the target module - not on any of the module's dependencies. The normal sublime text plugin loading logic loops through sys.modules and calls the plugin_loaded function (if the module has such a function).

randy3k commented 6 years ago

Maybe you are right. But it is too not straightforward to determine which modules should be reloaded, In addition, I don’t think we should reload any dependencies.

FichteFoll commented 6 years ago

(External) dependencies are not part of the package and should not be reloaded. When you modify those, you should probably del sys.modules['dep_name'] (and probably all submodules as well) in the console.

You could make a case for AutomaticPackageReloader also being able to reload a dependency iff you are working on one.

Varriount commented 6 years ago

@randy3k The modules that need plugin_loaded called are the same ones that are being reloaded. The work of finding and reloading them is already being done.

I've been able to patch AutomaticPackageReloader on my system to call plugin_loaded by using this patch:

175c176,179
<                 return module.__loader__.load_module(name)
---
>                 result = module.__loader__.load_module(name)
>                 if "plugin_loaded" in result.__dict__:
>                     result.plugin_loaded()
>                 return result

Although this code isn't quite right either - the modules loaded via reload_plugin have their plugin_loaded function being called twice.

randy3k commented 6 years ago

I would argue that NimLime should not modify the python search path for this modules. You should put a file in the root directory and import all the submodules to the root. In this way, it avoids having multiple plugin_loaded definitions and calls.

See for example how it is done in Terminus: https://github.com/randy3k/Terminus/blob/master/main.py

Varriount commented 6 years ago

@randy3k I've since modified NimLime to not modify the search path (look at the restructure branch).

randy3k commented 6 years ago

I guess you should also refactor this part of the codebase https://github.com/Varriount/NimLime/blob/restructure-for-st3/core/commands/__init__.py#L31 and import the submodules relatively. Why do you need this file at the first place?

Varriount commented 6 years ago

@randy3k That code loops through all submodules and exports the Sublime Text plugin classes. That way, when a new command or event listener is added to the commands directory, it is automatically picked up by Sublime Text via the star import in NimLime.py.

The import flow process looks something like this:

NimLime.py -- Star Import --> core/commands/__init__.py -- Dynamic Import --> core/commands/*.py

Is this causing some sort of problem with AutomaticPackageReloader? The dynamic import process uses the same import functions that Sublime's sublime_plugin.py plugin loader uses.

Thom1729 commented 5 years ago

Is this issue live, or does #20 resolve it?