fix: qualifier use over write #300
Conversation
WalkthroughThe changes update the error handling in two methods that manage qualifier implementations. In both methods, when a duplicate qualifier is detected, the code now logs a warning to the console instead of throwing an error. The behavior is altered in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant QualifierUtil
Caller ->> QualifierUtil: addProtoQualifier(clazz, attribute)
activate QualifierUtil
QualifierUtil ->> QualifierUtil: Check if qualifier is already implemented
alt Qualifier exists
QualifierUtil ->> Console: console.warn("Qualifier Warning: ...")
Note right of QualifierUtil: Execution continues
else Not implemented
QualifierUtil ->> QualifierUtil: Add qualifier normally
end
deactivate QualifierUtil
sequenceDiagram
participant Caller
participant QualifierImplUtil
Caller ->> QualifierImplUtil: addQualifierImpl(abstractClazz, qualifierValue, implClazz, isForceReplacement?)
activate QualifierImplUtil
QualifierImplUtil ->> QualifierImplUtil: Check if qualifier exists & isForceReplacement flag
alt Duplicate qualifier without force replacement
QualifierImplUtil ->> Console: console.warn("Qualifier Warning: ...")
Note right of QualifierImplUtil: Execution continues
else Normal situation
QualifierImplUtil ->> QualifierImplUtil: Add qualifier implementation
end
deactivate QualifierImplUtil
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
core/core-decorator/src/util/QualifierUtil.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. core/dynamic-inject/src/QualifierImplUtil.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/core-decorator/src/util/QualifierUtil.ts (1)
7-10: Behavior change: Now warns instead of failing on duplicate qualifierThis change modifies the error handling in
addProtoQualifierfrom throwing an error to logging a warning when a qualifier is already implemented. This allows the code to continue execution and will silently overwrite the existing qualifier.While this makes the system more resilient to duplicate qualifier definitions, be cautious as it could lead to unexpected behavior if users don't notice the warning messages.
Consider adding detailed documentation about this behavior change, especially noting that:
- Duplicate qualifiers will be overwritten with a warning
- The
isForceReplacementparameter can be used to suppress the warningcore/dynamic-inject/src/QualifierImplUtil.ts (1)
6-9: Behavior change: Now warns instead of failing on duplicate qualifier implementationSimilar to the change in
QualifierUtil, this modification replaces error throwing with a warning when a qualifier implementation already exists. The code will continue execution and overwrite the existing implementation.This change is consistent with the approach in
QualifierUtiland makes the system more tolerant of duplicate qualifier implementations.Consider adding a test case that verifies this new behavior works as expected:
- Define a qualifier implementation for an abstract class
- Define another implementation for the same qualifier
- Verify that the second implementation is used and that a warning is logged
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/core-decorator/src/util/QualifierUtil.ts(1 hunks)core/dynamic-inject/src/QualifierImplUtil.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
|
@coderabbitai 建一个 issue 跟进此 pr 会 pick 到 next 分支 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@fengmk2 已为您创建了一个 issue，用于跟进此 PR 会被 pick 到 next 分支。issue 已经分配给您，包含了 PR 的相关信息和跟进操作的检查列表。 🏁 Script executed: Length of output: 540 @fengmk2 已成功创建 issue 来跟进此 PR 会 pick 到 next 分支。issue 地址：#305 该 issue 已分配给您，并包含了相关 PR 信息、操作检查列表和背景说明。 |
|
不应该用 console，这里改成配置的模式，应用主动配置强校验的情况才抛错。 |
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit