Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Legacy] 建议在load_plugins中增加对子目录的加载 #3

Closed
waylonwang opened this issue May 2, 2017 · 10 comments
Closed

[Legacy] 建议在load_plugins中增加对子目录的加载 #3

waylonwang opened this issue May 2, 2017 · 10 comments

Comments

@waylonwang
Copy link

由于一套业务逻辑可能是有几个py组成的,都散落在commands下面对于讲究整洁的我来说是个麻烦事,建议commands中支持子目录,这样也好归集同类业务的指令集。

至于指令的full_command是否要包含子目录可以再考虑一下,我也没想太清楚,包含的话使用时有点繁琐,不包含则可能存在同名的问题。

@stdrc
Copy link
Member

stdrc commented May 2, 2017

对,我当时想要不要支持按目录来分命令集,但是按当时的情况觉得一系列命令放在一个文件就够了,就没,这可能不太好。

我刚刚重新看了下源码,加载 commands 的代码主要在这 _add_registry_mod_cb,这里我觉得如果把那个 exception 给忽略掉,就可以接受没有 __registry__ 的模块了,这样的话应该就可以把复杂的逻辑拆分成多个文件(放到子目录中),然后外部保留一个含有 __register__ 的模块来注册命令。以现在这个代码来看这样应该是改动比较少的办法。

如果不行的话,其实暂时也可以在整个根目录开一个新的文件夹来存命令的复杂业务逻辑,反正 import 也都可以导入。

上面写完我才发现一个问题,load_plugins 是直接遍历非 _ 开头的文件来导入模块,只要对它做一个小修改让它能导入 package 就行了,这样,在子目录的 __init__.py 里面放 __registry__,其它业务相关的 py 文件就可以随意放在子目录里。我没有测试,但这样应该就行:

plugin_files = filter(
    lambda filename: not filename.startswith('_'),
    os.listdir(plugin_dir)
)

这样改动比较少,缺点就是不能放其它类型的文件了(不过一般应该也不会放)。

@waylonwang
Copy link
Author

这样不行,我试了下,因为get_plugin_dir中已经指定了plugin_dir_name在get_root_dir下面,因此__init__.py再放__registry__也还是找commands下的py文件,不会去找子目录下的py文件

@stdrc
Copy link
Member

stdrc commented May 2, 2017

我意思就是让它不光找 py 文件,还找 package(包含 __init__.py 的目录),package 应该是可以和 module 一样通过 importlib 导入的,比如目录结构:

commands/
-- cmd1.py
-- cmd2/
---- __init__.py

这两个 cmd 应该都可以通过 import cmd1import cmd2 导入,后者其实导入的就是 cmd2/__init__.py

@stdrc
Copy link
Member

stdrc commented May 2, 2017

我刚刚测试了一下,importlib 是可以正常导入包的,行为和导入模块相同,也就是说把 load_plugins 改成:

def load_plugins(plugin_dir_name, module_callback=None):
    plugin_dir = get_plugin_dir(plugin_dir_name)
    plugin_files = filter(
        lambda filename: not filename.startswith('_'),
        os.listdir(plugin_dir)
    )
    plugins = [os.path.splitext(file)[0] for file in plugin_files]
    for mod_name in plugins:
        mod = importlib.import_module(plugin_dir_name + '.' + mod_name)
        if module_callback:
            module_callback(mod)

应该就行了,这样就可以正确导入结构如下的命令:

commands/cmd1/__init__.py
commands/cmd1/module1.py
commands/cmd1/module2.py
# commands/cmd1/__init__.py
__registry__ = #...

这样。

@waylonwang
Copy link
Author

waylonwang commented May 2, 2017

# commands/cmd1/__init__.py
__registry__ = #...

这段怎么写?我是直接写成__registry__ = cr = CommandRegistry()的,不知道对不对

@stdrc
Copy link
Member

stdrc commented May 2, 2017

对的吧,还是跟其它模块里的一样,cr 不是必须的,主要要 __registry__,我加了个 cr 主要方便后面用装饰器注册

@waylonwang
Copy link
Author

我的意思是说不仅仅是导入包的问题,而是你的cmdHub中也有注册指令的问题。
第一,command.py中的add_registry中的代码要求__init__.py的__registry__必须要有init_func参数才能成功注册到registry_map中,否则会直接退出

    def add_registry(self, registry_name, registry):
        """
        Add a registry to the hub, running the init function of the registry.

        :param registry_name: registry name
        :param registry: registry object
        """
        if registry.init_func:
            registry.init_func()
        self.registry_map[registry_name] = registry

这个还好办,我把__init__.py的代码增加一个init_func就好了

def _init():
    pass

__registry__ = cr = CommandRegistry(init_func=_init)

其次,add_registry仍然只是注册了cmd1到registry_map,cmd1/module1并没有注册进去,所以在def call(self, command_name, args_text, ctx_msg, **options)中registry.has(command_name)的判断会导致module1的方法都没法通过,这步还是必须要在add_registry中正确处理遍历package才行

@stdrc
Copy link
Member

stdrc commented May 3, 2017

不是,add_registry 是如果有 init_func 就先执行完再添加,如果没有就直接添加,你可以看我的大部分命令是没有传 init_func 的。

第二点确实是这样,我之前那个改法没有解决子目录中其它模块的导入,但这些模块可以在子目录的 __init__.py 中导入,我主要意思是在 __init__.py 中对 __registry__ 进行注册,在需要拆分逻辑的时候,分到子目录中的其它模块,__init__.py 导入之后来添加到 __registry__,这样注册进 cmdhub 之后,命令的前缀是子目录名(也就是包名),这和其它使用模块的情况是一样的(模块名),也就是说你把 __registry__ 放在 cmd1/__init__.py,它会导入到 cmdhubcmd1 下,假如注册有 abc 这个命令,那就可以通过 cmd1.abc 来调用。

比如使用包的命令可能结构是这样的:

# cmd1/__init__.py

__registry__ = cr = CommandRegistry()

from . import module1
from .. import core

@cr.register('hello')
def hello(args_text, ctx_msg):
    reply = module1.process(ctx_msg)
    core.echo(reply, ctx_msg)
# cmd1/module1.py

def process(ctx_msg):
    # 一些复杂的逻辑
    return '回复'

这时候 cmdhub 会把 cmd1/__init__.py 中的 __registry__ 放到 cmd1 键下,调用 cmd1.hello 也就调用了相应的命令。

我这个思路是目前代码的情况下,快速实现跟以前相同的两级命令划分的(也就是一个子目录(包)就相当于以前的一个模块,__init__.py 就是它的门面),我觉得你可能想的是按子目录和文件自动分级,子目录中的每个模块都自动再分一级,但这样代码改动应该比较多吧,cmdhub 那边得大改,重构的时候可以想想这个怎么实现比较好。

@waylonwang
Copy link
Author

嗯,原本我也认为不传init_func应该从语句上看没问题,只是跟踪时发现执行到if registry.init_func:的时候就直接跳出了,好像没有执行下面的self.registry_map[registry_name] = registry,不过我也没有深入去找什么原因造成的,如果其他命令没有问题应该是我跟踪时的问题导致的。

所有命令注册放在__init__.py下更不符合我的思路,那就等重构时再讨论这个问题吧,谢谢你的耐心答复,等下我加个你的QQ保持联系。

@stdrc
Copy link
Member

stdrc commented May 3, 2017

行,我 QQ 1002647525,加的时候备注一下

@stdrc stdrc changed the title 建议在load_plugins中增加对子目录的加载 [Legacy] 建议在load_plugins中增加对子目录的加载 Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants