-
Notifications
You must be signed in to change notification settings - Fork 17
Update #9
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
Conversation
Aalivexy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果你要PR的话请一个功能发一个PR,并且保证编译通过
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
你换成相对路径这样别人编译的时候是找不到的
|
outlook给我邮件全干垃圾箱了,啧 |
This reverts commit a3be385.
| text: String, | ||
| from: String, | ||
| to: String, | ||
| result: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议你先review一下代码,不要vibe完能跑就行,加这玩意真是不明所以
如果你希望有个啥新功能可以开个issue说一下为什么想要,我来做就行,这些commit真是污染git历史......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这些改动是为了兼容 mtranserver 的接口,commit message 里写了。
并非 “不要 vibe 完能跑就行,加这玩意真是不明所以”
| const ENV_API_KEY: &str = "API_KEY"; | ||
| const ENV_LOG_LEVEL: &str = "RUST_LOG"; | ||
|
|
||
| // Environment variable aliases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为什么要加这些环境变量?我没看懂要兼容什么?
| Ok(models) | ||
| } | ||
|
|
||
| async fn shutdown_signal() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不同的功能不要在同一个PR实现,没法review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rust 相比原版缺少了关闭服务器的响应事件
| let server_ip = std::env::var(ENV_SERVER_IP).unwrap_or_else(|_| "127.0.0.1".to_string()); | ||
| let server_port = std::env::var(ENV_SERVER_PORT) | ||
| .ok() | ||
| let server_port = get_env_with_alias(ENV_SERVER_PORT, ENV_SERVER_PORT_ALIAS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这环境变量alias真是污染代码啊
| let from_code = get_iso_code(&source_lang)?; | ||
| let to_code = get_iso_code(&target_lang)?; | ||
|
|
||
| // If source and target languages are the same, return the original text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不同功能请不要在同一个PR实现
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同语言直接返回
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这测试我觉得是真没什么用,要说覆盖率它基本没啥覆盖,要说跑通它也没有全流程跑通
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
调试用的,可以直接删掉,或者你继续增加全覆盖的测试
|
你要不然和我说你想要啥吧,这PR没法合并,不然commit历史会一团糟 |
|
兼容 mtranserver 的接口和环境变量,还有一些都写在注释里了。不想要的你可以直接编辑 PR |
|
如果你有更好的兼容方式,可以加上,环境变量用别名的方式确实不太优雅。 |
问题是我没看懂要兼容谁
这俩本来也不能直接替换,模型不一样啊,linguaspark默认是不捆绑模型的,如果你希望容器级别的默认模型+兼容,那应该改的是dockerfile/compose文件,而不是代码 |
|
行 |
@Aalivexy