-
Notifications
You must be signed in to change notification settings - Fork 98
fix: refactor config type #476
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
| } | ||
| export enum SyncDeleteMode { | ||
| ignore = 'ignore', | ||
| block = 'block', |
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.
这是一个 TypeScript 模块。代码本身很短,只有一些枚举。代码看起来没有风险。
下面是一些可以改进的建议:
- 没有必要添加注释
export enum SyncMode等。 - 对于每个枚举值,请考虑是否使用 PascalCase(例如
Existing代替exist)。
| cnpmcore: typeof cnpmcoreConfig; | ||
| cnpmcore: CnpmcoreConfig; | ||
| } | ||
| } |
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.
此代码补丁主要是将导入的模块变量名、接口类型等适当修改。在修改前后的代码中,只有引用某个配置的变量声明和接口定义被改动。因此,代码风险较低。
针对这个补丁的改进建议如下:
- 如果 cnpmcoreConfig 配置仅作为类型定义使用,则可以直接删除相关的 import 和 typeof 引用,在 EggAppConfig 中直接使用一个与 CnpmcoreConfig 结构一致的对象字面量。这样可以减小代码复杂度。
- 如果 cnpmcore 配置是可变的,那么应该考虑通过配置文件实现动态加载,而不是直接放在源码中。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #476 +/- ##
==========================================
- Coverage 97.81% 97.39% -0.43%
==========================================
Files 152 168 +16
Lines 13549 15680 +2131
Branches 2107 2012 -95
==========================================
+ Hits 13253 15271 +2018
- Misses 296 409 +113 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
| export enum SyncDeleteMode { | ||
| ignore = 'ignore', | ||
| block = 'block', |
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.
这段代码是对枚举类型进行了定义和扩展。从代码来看,似乎没有明显的错误风险。
以下是一些改进建议:
- 考虑给枚举类型的值加上注释,方便他人理解使用。
- 可以考虑将变量命名更有意义和语义化,易于阅读和理解。
- 对于某些枚举类型,可能需要添加更多的选项,以应对潜在的需求变化。
祝你好运!
| cnpmcore: typeof cnpmcoreConfig; | ||
| cnpmcore: CnpmcoreConfig; | ||
| } | ||
| } |
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.
这是一段 TypeScript 代码,它主要做了一个导入修改和一个类型修改。
将原本导入的 { cnpmcoreConfig } 修改为了 { CnpmcoreConfig },这可能意味着 config.ts 文件中的 cnpmcoreConfig 已被重命名为 CnpmcoreConfig。需要确认该更改是否正确。
在 EggAppConfig 接口中,cnpmcore 属性的类型也由原来的 typeof cnpmcoreConfig 修改为了 CnpmcoreConfig 类型。需要确认 CnpmcoreConfig 类与 cnpmcoreConfig 值之间的类型兼容性,如果存在问题,则可能需要调整相应的类型定义。
代码挂载还未完成,我们检测不出其余的潜在风险。
| const type = changesStreamRegistryMode === ChangesStreamMode.json ? RegistryType.Cnpmcore : RegistryType.Npm; | ||
| const registry = await this.createRegistry({ | ||
| name: PresetRegistryName.default, | ||
| type, |
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.
这段代码主要的改动是将 PresetRegistryName 导入了一个新的常量 ChangesStreamMode,同时在某个函数中使用 changesStreamRegistryMode 去设置 type 属性。目前没有明显的 bug 风险,但以下是几点建议:
- 添加注释来解释为什么需要使用
ChangesStreamMode.json更改type。这样会让未来的维护者更好地理解代码。 - 当调用
createRegistry()函数时,确保输入参数的合法性。
fengmk2
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.
+1
[skip ci] ## [3.22.1](v3.22.0...v3.22.1) (2023-05-25) ### Bug Fixes * refactor config type ([#476](#476)) ([ebc8c98](ebc8c98))
|
🎉 This PR is included in version 3.22.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ChangesStreamModeenums.ChangesStreamMode枚举