trexinc / evil-programmers

Misc. Far plugins and tools by evil programmers
66 stars 26 forks source link

Support importing plugin's sibling modules #50

Closed vladfolts closed 3 years ago

vladfolts commented 3 years ago

E.g.: Far Manager\Plugins\plugin1 plugin1.far.py <- "import plugin1_module2" plugin1_module2.py

vladfolts commented 3 years ago

I'm not really happy with this solution. Maybe you can suggest a better way to import siblings.

alabuzhev commented 3 years ago

I vaguely remember that touching sys.path is not the best way to do it, there's even a commit somewhat related to it (6a561835fbd944bf2d24e0ce3a8906bc9fea59f6), but I don't really remember why, so... I don't mind merging it as long as it works.

Just keep the formatting consistent please (spaces/tabs etc.).

vladfolts commented 3 years ago

I'll fix the formatting. I just thought that the Spaces have won already :)

vladfolts commented 3 years ago

As for python imports - ideally, I would like to make the import work as if the plugin is a python3 package so "." would work:

from . import plugin1_module2

I just have no experience with python embedding...

vladfolts commented 3 years ago

As for C++ formatting - I have some C++ changes I intend to push eventually (mostly about Panel API bindings). The current C++ formatting seems somewhat unusual to me (especially local variable names starting with a capital letter). Do you mind if I'll use more traditional formatting like underscores and "_t" suffixes for classes?

vladfolts commented 3 years ago

Just keep the formatting consistent please (spaces/tabs etc.).

Fixed.

alabuzhev commented 3 years ago

Do you mind if I'll use more traditional formatting like underscores and "_t" suffixes for classes?

Please keep it consistent with the existent formatting. Having it different here and there doesn't really help. _t is ok to resolve name conflicts, otherwise there's not much need in it.

alabuzhev commented 3 years ago

Maybe you can suggest a better way to import siblings.

I think I found a better way (ff209fb18d3a54ea9b012d9a309e46c759a20e91):

vladfolts commented 3 years ago

I think I found a better way (ff209fb):

Great, thanks!

vladfolts commented 3 years ago

I think I found a better way (ff209fb):

Great, thanks!

I tried this and it's half-baked: the framework expects FarPluginClass in the loaded module (which is __init__.py in our case). But I cannot import plugin.far.py in __init__.py to expose FarPluginClass because its name contains a dot ('.'). So the current "package" working schema I came up with looks like:

__init__.py

from .plugin_impl import Plugin as FarPluginClass

plugin.far.py is empty and plugin_impl.py is the plugin's implementation

Having "special" empty plugin.far.py seems unnecessary.

alabuzhev commented 3 years ago

I cannot import plugin.far.py in __init__.py to expose FarPluginClass because its name contains a dot ('.')

You can put the implementation into __init__.py. You can put it into any other "importable" file in that "package". It doesn't have to be in *.far.py.

Having "special" empty plugin.far.py seems unnecessary.

We need a way to recognise "it is a Far plugin, load it" and "it is a regular Python package / file, leave it alone".

vladfolts commented 3 years ago

To clarify: I don't think it is critical but it is at least non-intuitive. We already have special plugin.far.py and special FarPluginClass - OK. I'm adding __init__.py and expect things to continue working as before (plus relative import to work as well).

alabuzhev commented 3 years ago

Far knows nothing about modules, packages or the Zen of Python. It just throws at us every single file under Plugins, asking "can you load it?" We can try to load every single *.py file and look for a special class in it indeed. And this is how it was initially. If you have a single helloworld plugin in just one py file, it's not a problem at all. But if your plugin has multiple modules, dependencies in the same directory, dependencies of those dependencies etc. it would be a performance disaster. Having a file with a special name *.far.py effectively narrows it down to one load only: either that *.far.py file itself, or the __init__.py in the same directory, if it exists.

vladfolts commented 3 years ago

It is fine to have one special *.far.py. My concern was about the confusion on adding __init__.py. Instead of extending the behavior (start allowing the relative import) we are changing the behavior (start requiring __init__.py to export FarPluginClass instead of *.far.py). I thought that the possible "fix" would be as simple as continue to load *.far.py after loading __init__.py first (just to make python believe that this is a package). If there are technical difficulties - OK, I'm fine with leaving it as is.

alabuzhev commented 3 years ago

I thought that the possible "fix" would be as simple as continue to load *.far.py after loading__init__.py first (just to make python believe that this is a package)

Hmm, I guess we could do that, but would it be idiomatic / pythonic? If I put __init__.py and script.py to the same directory and execute script.py, Python won't automatically execute __init__.py, right?

vladfolts commented 3 years ago

I think it won't unless you import script.py (from another script). If you think the proposed "fix" is confusing as well - I won't insist on it. From my personal perception as a newcomer without any special knowledge of Far specifics - seeing __init__.py in a Far's plugin directory would definitely suggest that this __init__.py is going to be loaded first. Afterward, I could learn about special *.far.py and FarPluginClass from examples.

alabuzhev commented 3 years ago

I think it won't unless you import script.py (from another script).

I see what you mean. Ok, treating it "as-if" Far "imports" *.far.py is an interesting approach, let's explore it. 5a53e2c4ca9c7daacb443528227c8ae3e65b50a3

vladfolts commented 3 years ago

So now it is possible not only to import relatively but also to use plugin's folder name as a package name to import from? I.e. it would be possible to import from other plugins?

OK, for my use case it works, I don't have any objections, thanks again!

alabuzhev commented 3 years ago

So now it is possible not only to import relatively but also to use plugin's folder name as a package name to import from?

Yep. I'm not sure it's a good idea, but... We will see.

I.e. it would be possible to import from other plugins?

It was probably already possible.