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

WIP: feat: use deeplx to translate #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

so2liu
Copy link

@so2liu so2liu commented Apr 22, 2024

可以正常搜索翻译,但是 action 是空的。那个ListDisplayItem还在研究,感觉好复杂呀,大佬你写的代码是不是有点儿太绕了,感觉没必要做这么多抽象

#56

image

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello so2liu, Thank you for your first PR contribution 🎉 so2liu

@@ -99,7 +99,7 @@ export class AppKeyStore {
static volcanoSecretKey = myPreferences.volcanoAccessKeySecret.trim();

static openAIAPIKey = myPreferences.openAIAPIKey.trim();
static openAIEndpoint = myPreferences.openAIAPIURL.trim() || "https://api.openai.com/v1/chat/completions";
static openAIEndpoint = myPreferences.openAIAPIURL?.trim() || "https://api.openai.com/v1/chat/completions";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里 myPreferences.openAIAPIURL 需要加 ? 吗

MyPreferences 定义

openAIAPIKey: string;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typescript的定义只是提示,不影响真实运行中的值是不是undefined。我开发过程中刚启动的时候会报这个read property from undefined错误,我才加这个的。反正加了没什么缺点

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok,ts 我是野生实战派,水平有限,只要求代码能跑起来就行,没怎么注意代码细节 😥

期待你能优化改进这些问题。

@@ -274,6 +289,9 @@ export default function (props: LaunchProps<{ arguments: EasydictArguments }>) {
</List.Section>
);
})}
<List.Section title="DeepLX">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不不,不要这样写死固定一个 DeepLX 服务放这里,我们应该要让用户选择开启或关闭这些服务,具体你可以看一下设置页,里面甚至还支持服务顺序排序。

本来我希望直接复用已有的 DeepL 服务,只是对它进行一些优化,如果用户没有填写 DeepL API URL,就直接调用网页 API,这样实现起来最简单,你都不用改其他地方的代码。

如果你想添加一个单独的 DeepLX 也行,那就参考项目中已有的新增服务流程。

image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好的,我再重新改一下

@tisfeng
Copy link
Owner

tisfeng commented Apr 23, 2024

不需要显示翻译原文。

image

@tisfeng
Copy link
Owner

tisfeng commented Apr 23, 2024

中文翻译有问题。

image

@tisfeng
Copy link
Owner

tisfeng commented Apr 23, 2024

如果译文过长,应该使用 detail 页面显示的,具体请看代码。

image image


export const requestDeepLXTranslate = async (queryWordInfo: QueryWordInfo): Promise<{ translations: string[] }> => {
const body = await axios.post<{ data: string }>("https://deeplx.mingming.dev/translate", {
text: queryWordInfo.word,
Copy link
Owner

@tisfeng tisfeng Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

虽然这是一个取巧的方式,但这样并不好。

  1. 这样依赖了一个别人提供的网络服务,这个服务不能保证一直能用。
  2. 这个服务也是使用 DeepL 网页接口,Raycast-Easydict 有几千用户,高频使用很可能会导致接口被 DeepL 官方封 IP。
  3. 即使是这个接口保证能使用,但毕竟是第三方的野生接口,部分用户会有隐私担忧。

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

其实我也搭了一个这样的 DeepL 接口,但一直没有直接开放给用户,就是有这些方面的担忧。

所以,这里优先直接使用网页接口,使用用户自己的 IP 去请求,这样即能分担 IP 压力,也不会有隐私担忧。

具体实现可以用 https://github.com/ifyour/deeplx 这个库,如果直接使用不方便,那可以参考它的 ts 代码也行 https://github.com/ifyour/deeplx/blob/main/src/query.ts

Copy link
Owner

@tisfeng tisfeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

可以正常搜索翻译,但是 action 是空的

这个什么意思,没明白 😑

感觉好复杂呀,大佬你写的代码是不是有点儿太绕了,感觉没必要做这么多抽象

复杂吗,有可能,毕竟我对 ts 和 react 不熟悉,毕竟这是我第一个前端项目,看了一周 TS 入门教程,然后基于这个开源项目 raycast-Parrot 做的,代码很丑陋也正常。

虽然代码比较糟糕,功能比较多、比较杂,但这个项目我还是花了心思的,一些细节设计我觉得还是可以的。

如果你想优化或重构代码,我是非常欢迎的,但建议你先多使用它,熟悉一下已有的各项功能,然后在保持这些功能不被破坏的情况下进行重构优化。

如果你对一些功能不了解,或者有不同看法,或者想修改它,可以先开个 issue 我们讨论一下。

@tisfeng
Copy link
Owner

tisfeng commented Apr 23, 2024

如果要重构,我建议可以先重构翻译服务结构,参考我另一个项目 Easydict ,写一个翻译服务基类,定义好各种协议方法,如服务名字,图标,支持的语言,翻译接口 translate 方法等,然后每个翻译服务都是一个子类重写就可以,这样结构会好很多。

例如这个 彩云小译 代码实现。

@so2liu so2liu changed the title feat: use deeplx to translate WIP: feat: use deeplx to translate Apr 24, 2024
@so2liu
Copy link
Author

so2liu commented Apr 24, 2024

可以正常搜索翻译,但是 action 是空的

这个什么意思,没明白 😑

感觉好复杂呀,大佬你写的代码是不是有点儿太绕了,感觉没必要做这么多抽象

复杂吗,有可能,毕竟我对 ts 和 react 不熟悉,毕竟这是我第一个前端项目,看了一周 TS 入门教程,然后基于这个开源项目 raycast-Parrot 做的,代码很丑陋也正常。

虽然代码比较糟糕,功能比较多、比较杂,但这个项目我还是花了心思的,一些细节设计我觉得还是可以的。

如果你想优化或重构代码,我是非常欢迎的,但建议你先多使用它,熟悉一下已有的各项功能,然后在保持这些功能不被破坏的情况下进行重构优化。

如果你对一些功能不了解,或者有不同看法,或者想修改它,可以先开个 issue 我们讨论一下。

@tisfeng 感谢大佬写了这么多意见。我主要是发现有两个问题,一个是没有用async/await,全部使用Promise的callback,导致逻辑比较绕;一个是没有用现有react的各种生态。这个PR我再改一改哈

@tisfeng
Copy link
Owner

tisfeng commented Apr 24, 2024

ok,代码层面请放心大胆优化,这个你应该比我了解,就不多说了。

另外,这个 PR 是实现 DeepL 网页接口,如果你想先做一些其他的代码结构优化,建议另外开 PR。

建议每个 PR 都只做一件事,保持简洁,如果一个 PR 分多个步骤,也要拆分为多个单独的 commit,这样我 review 也轻松点。

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

Successfully merging this pull request may close these issues.

None yet

2 participants