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

feat: optimize ICompletionItem type and add ICompletionList type #102

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

HaydenOrz
Copy link
Collaborator

简介

优化自动补全相关接口 #99

主要变更

  1. 重新定义 ICompletionItem 接口,这个接口与 monaco 内置的 CompletionItem 几乎完全相同,区别是 ICompletionItemrangeinsertText 是可选的,因为 Monaco SQL Lanuages 在内部处理了这两个烦人的属性
  2. 新增 ICompletionList 接口,这个接口与 monaco 内置的 CompletionList 几乎相同,区别是ICompletionListsuggestions 属性的类型为 ICompletionItem[]
  3. compeltionService 返回值的类型同步变更

Why

在 PR #100 中支持了自动补全功能返回值中的 incomplete 属性,这是一个很好的PR。

这个PR 也从侧面说明自动补全的相关逻辑可能封装的痕迹过重,我希望 competetionService 的返回值尽量与 monaco 内置的 provideCompletionItems 返回值类型保持一致,这可以降低 Monaco SQL Languages 的上手成本。

另外在此之前,ICompletionItem 接口的 detail 成员被设置为必填,但实际上这是不必要的

@HaydenOrz HaydenOrz added the improvement Improve existing feature label Jan 26, 2024
@Kijin-Seija
Copy link
Contributor

感谢您这边的更新,修复了我之前代码风格上不一致的地方,同时还额外把dispose这个方法也导出来了。之前只考虑了当前需求。确实应该同步provideCompletionItems的所有配置。
关于自动补全的相关逻辑,个人觉得也许可以考虑提供一个开关把生成的provideCompletionItems交给用户自己去调用monaco.language.registerCompletionItemProvider去处理,这样可以让本身有一些配置的开发者能够和自己的配置合并。

@HaydenOrz
Copy link
Collaborator Author

关于自动补全的相关逻辑,个人觉得也许可以考虑提供一个开关把生成的provideCompletionItems交给用户自己去调用monaco.language.registerCompletionItemProvider去处理,这样可以让本身有一些配置的开发者能够和自己的配置合并。

这是个值得考虑的问题,目前这样做主要是因为,自动补全的 registerCompletionItemProvider 中需要与 worker 进行通信,即需要通过 dt-sql-parser 获取必要的语法解析信息,而个人不是很想把 worker 相关的接口直接暴露给用户,因为可能导致很多意想不到的问题。

@HaydenOrz HaydenOrz merged commit 00c7c6b into DTStack:main Jan 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improve existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants