[code review] Tim 对 dev 分支 的 code reivew #1
Conversation
There was a problem hiding this comment.
一些疑惑和注意到的点:
- 注意到主项目中用 python 标准库的 logging 替换了 loguru，这个变化有什么特殊考量吗?
- 这个代码库中有非常多的 type 问题。请把你 cursor 或 vscode 的 pylance 或其他 type checker 的严格程度从 none 改成 basic。
uv run ruff check也有不少问题。
感觉直接问你比较方便的问题:
- 如果我现在要开始写 OLV main，OLV main instance 的入口在哪里？我该怎么拿到那些 instance 的实例？
- (Update) 注意到了 OLV 后端架构的设计文档，我理解了。我们 ws 是单独的服务器进程，需要单独启动？
一些评论
- 不要用 AI 写架构代码，就算用 AI 写完代码，你也得做 code review
- 你可以用 AI 润色文档，但你得先自己写啊。就算你真的一定要用 AI，你写完也得自己看过一遍。因为这个文档有问题。先不说车轱辘话说一堆，有些命令根本就是错的啊
- 其实用不用 AI 写代码我也管不了这么多。不论用不用 AI，你 push 上来的代码你是要负责的。当我们质问你代码里做的设计决定时，你最好有个足够好的理由。
- docstring 写的明白一些。函数是做什么的，应该要怎么用，不能怎么用，参数是什么意思，回传值又是什么意思，还有他们的类型，写清楚。要以 "看了 docstring 之后，调用这个函数时就不用看具体实现细节了" 的预期去写 docstring。
- 代码里面一堆 type error。
- 我意识到你可能没懂什么该写在文档里，什么该写在 docstring 里。
- 实现细节 (implementatino details)，比如函数用处，变量的明确意义，包括应该怎么用，不应该怎么用，因为可能会改，而且与实现细节高度相关，写在 docstring 里面比较合适。
- 对于公共的，概念性的，API 性质的，请写在文档里。
- 目前在 olv_main 里面的代码应该都是旧的类型定义，马上我写新的交互模式的过程中可能会改那部分的东西。
还好，问题不大。注意一下就行。我下面的 review message 有些可能情绪比较激动，不用在意。
|
|
||
| ## 配置文件设置 | ||
|
|
||
| ### 创建配置文件 |
There was a problem hiding this comment.
实际上，代码中没有任何部分读取了环境变量，起码没让 .env 文件起作用。
我们在 uv 环境中安装了 python-dotenv 却没有在代码中的任何地方使用到这个库？
又或者是我看漏了什么东西吗？
确实是看漏了。.env 的读取是用 pydantic 的 SettingsConfigDict 实现的，所以我们不需要在 pyproject.toml 里面明确安装 python-dotenv 库？
文档感觉可以说一下 .env 的读取是 pydantic 做的。
| self._config = config.copy() | ||
|
|
||
|
|
||
| class ASREngineError(Exception): |
There was a problem hiding this comment.
这个之后 types 相关的我们可以弄成一个单独的代码文件，不过没关系，不重要。
| class PluginConfig(BaseModel): | ||
| """Plugin configuration model""" | ||
|
|
||
| version: str = Field(..., description="Plugin version") |
There was a problem hiding this comment.
plugin config 跟 plugin info 都有 plugin version? 为什么呢？没懂喵。
| self.plugin_statuses[plugin_name] = PluginStatus.STARTING | ||
|
|
||
| # Check if plugin has a predefined service_url (like localhost:8080) | ||
| if plugin_info.service_url and plugin_info.is_local: |
There was a problem hiding this comment.
看到这里脑子卡壳了一下。local 插件不是没有 service_url 吗？
为什么这里要判断 is_local 还要判断 service_url 存在？
所以 local 插件有可能会有 service_url?
这个 is_local 的 local 指的究竟是什么？是本地运行的插件还是由 olv launcher 拉起的插件？在 docstring 里面写清楚。
目前的逻辑应该是 local 也包含运行在 localhost 上的服务器，这包含用户自己运行的，不是 OLV launcher 拉起的服务器? 感觉有点怪。跑在 localhost 上面的 plugin，依照这种逻辑，有可能不是 OLV launcher 拉起来的，也有可能 (举个例子) 是跑在别的容器里面的。这跟 remote 有啥区别？而且后面有个地方用判断完 is_local 了之后尝试处理 pyproject.toml? 这对吗？
我看了一下，plugin_info.service_url 应该是个不可变的参数，所以这个 and plugin_info.is_local 应该是个完全没用的判断，因为 is_local 的判断本身就依赖于 service_url 的判断，并且 这块代码属于 _start_local_plugin，应该只会在 plugin 是 local 的时候被执行。
| return True # No service_url means local | ||
|
|
||
| # Parse URL to check if it's localhost | ||
| parsed = urlparse(self.config.service_url) |
There was a problem hiding this comment.
所以运行在 localhost 上面的插件也被认作是 local? 那运行在 localhost 上但不是我们项目自己拉起来的 plugin 也会判断成 local 吗？
这个 local 是指跑在同台机器上的插件，还是由 OLV launcher 拉起来的插件，这点要在 docstring 里面写的很明白。之后这种关键细节都要写在 docstring 里面，不然肯定有人会搞错。
我认为 local 应该要明确的被定义为 由 OLV launcher 拉起的本地插件，但我不确定你的想法。你文档里也没写你对这个东西的定义，很显然你注释里也没写，所以我不确定你的设想是什么。
- local 插件的生命周期一定由 OLV launcher 管理吗？
- local 插件的代码目录存放 OLV launcher 可以访问的目录吗？
- 是跑在 localhost 上的插件都属于 local 插件吗？
不论我们最终结果是什么，都要明确的写在 docstring 和注释中。
- built the skeleton for the websocket server for olv_main
- add "launcher_url" configuration to olv_main so that the server knows the url to connect to olv_launcher - add test to olv_main config with pytest
Co-authored-by: Yi-Ting Chiu <mytim710@gmail.com>
|
这个 PR 的问题欢迎大家合作修改，在 dev commit 就行，新发现的问题也可以通过 comment 加。 |
chat 和 room 之后 review 一下，character 已经 review 完毕。 reference 就不用在意，都是临时文件
2 participants
一般我们可能会等到合并到 main，代码相对完整的时候开 pull request 再来做 code review，但我在看代码的时候有不少疑问，注意到了不少问题，用pull request 的 code review 工具做 code review 我个人认为比较方便，就先开了个 pr。
这个
code-review/dev-06-04是基于 main 的一个分支，也就等于什么都没有。review 的代码是目前的 dev 分支work in progress...
(Update)
review 的差不多了。有些代码我没有读的特别细 (主要有点累)，但发现了不少问题，不过不能排除是我自己没读懂代码，造成了误解。请看一下。