fix: add qualifier check #295
Conversation
WalkthroughThis pull request updates the Changes
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
ERR_PNPM_OPTIONAL_DEPS_REQUIRE_PROD_DEPS Optional dependencies cannot be installed without production dependencies ✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
PR Overview
This pull request improves qualifier handling by introducing an optional force replacement parameter to prevent duplicate qualifier errors when explicitly intended.
- Added an optional isForceReplacement parameter to both QualifierImplUtil.addQualifierImpl and QualifierUtil.addProtoQualifier
- Updated QualifierImplDecoratorUtil and AppLoadUnit to propagate the new parameter and override behavior accordingly
Reviewed Changes
| File | Description |
|---|---|
| core/dynamic-inject/src/QualifierImplUtil.ts | Updated addQualifierImpl to include an optional force replacement parameter |
| core/core-decorator/src/util/QualifierUtil.ts | Updated addProtoQualifier to include an optional force replacement parameter |
| core/dynamic-inject/src/QualifierImplDecoratorUtil.ts | Modified the decorator generator to forward the new parameter |
| plugin/tegg/lib/AppLoadUnit.ts | Changed addProtoQualifier call to force replacement in default qualifiers |
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
core/dynamic-inject/src/QualifierImplUtil.ts:7
- Consider adding tests to verify that when isForceReplacement is true, the duplicate qualifier error is not thrown.
if (QualifierImplUtil.getQualifierImp(abstractClazz, qualifierValue) && !isForceReplacement) {
core/core-decorator/src/util/QualifierUtil.ts:8
- Consider adding tests to ensure that passing a true value for isForceReplacement bypasses the duplicate qualifier error.
if (QualifierUtil.getQualifierValue(clazz, attribute) && !isForceReplacement) {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/types/dynamic-inject.ts (1)
8-8: Good addition of the optional parameter.The addition of the
isForceReplacementoptional parameter enhances the flexibility of theImplDecoratortype. This allows for explicit control over whether existing qualifier implementations should be replaced, which helps prevent accidental overrides while still allowing intentional replacements when needed.Consider adding JSDoc comments to document this parameter's purpose and behavior to improve code readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)core/types/dynamic-inject.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (javascript)
|
@akitaSummer pick 到 next 分支？😄 |
|
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes
Documentation