fix: remove duplicated packages #299
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Outside diff range and nitpick comments (2)
test/index.js (2)
50-51: Translate comment to English for better accessibility.Consider translating the Chinese comment to English to improve accessibility for international contributors. This aligns with best practices for maintaining open-source projects.
Here's a suggested translation:
-// 解析 allowPackages 的每个包的组织和包名，如果该组织已存在 allowScopes 中，则不需要再添加具名包 -// 例如：@babel 已存在，那么 @babel/core 就不需要再添加 +// Parse the organization and package name for each package in allowPackages. +// If the organization already exists in allowScopes, there's no need to add the named package. +// For example: If @babel already exists, then @babel/core doesn't need to be added.
66-72: Improve error message and provide resolution hint.The current implementation logs the duplicates but doesn't provide a clear error message or guidance on how to resolve the issue. Consider enhancing the error reporting to make it more actionable for developers.
Here's a suggested improvement:
if (duplicatedPackages.size > 0) { console.log('Duplicated packages in allowScopes:'); for (const [scope, packages] of duplicatedPackages) { console.log(` - ${scope}: ${packages.join(', ')}`); } - assert.fail('Duplicated packages in allowScopes'); + assert.fail( + 'Duplicated packages found in allowPackages for scopes already in allowScopes.\n' + + 'To resolve this, remove the following packages from allowPackages:\n' + + Array.from(duplicatedPackages.entries()) + .map(([scope, packages]) => ` ${scope}: ${packages.join(', ')}`) + .join('\n') + ); }This change provides a more detailed error message and explicitly lists the packages that need to be removed to resolve the duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- package.json (0 hunks)
- test/index.js (1 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
🔇 Additional comments (1)
test/index.js (1)
52-73: LGTM with minor improvements.The overall implementation of this test case is well-structured and effectively checks for duplicates between
allowPackagesandallowScopes. It aligns with the PR objectives and enhances the validation logic of the package configuration.With the suggested improvements for the comment translation and error messaging, this addition will significantly contribute to maintaining a clean and efficient package allowlist.
[skip ci] ## [1.134.1](v1.134.0...v1.134.1) (2024-10-11) ### Bug Fixes * remove duplicated packages ([#299](#299)) ([e59dcca](e59dcca))
白名单解析规则是 先 scope 再看 pkg，已经存在于白名单的 scope 则不再需要添加具名的 pkg
Summary by CodeRabbit
Chores
Tests