Conversation
leaperone-bot
left a comment
There was a problem hiding this comment.
🤖 Code Review 🟡
PR #27 代码审查:fix(tag): add tag name validation
整体评价
PR 实现了 tag 名称的基本验证,包括长度限制、字符白名单和保留名 latest 的确认提示。代码结构清晰,与现有风格一致。但有以下几点值得讨论:
1. Issue #19 要求未完全覆盖
Issue #19 明确提到需要校验以下内容:
- ✅ 限制 tag 长度
- ✅ 禁止特殊字符
⚠️ 校验保留名 — 仅处理了latest,未处理 issue 中提到的init等其他保留名⚠️ 禁止..— issue 特别提到..,但正则^[a-zA-Z0-9._-]+$允许..(虽然点号已被允许,..作为 tag 名可能引发路径遍历类问题,取决于后端存储实现)
建议:考虑增加对 .. 和其他保留名(如 init)的校验,或在 issue 中说明不需要。
2. 验证顺序的细微问题
当前验证逻辑放在 trimmedTagname 之后、已存在检查之前,这是合理的。但 latest 确认提示使用了 inquirer.prompt,如果在非交互式环境(CI/管道)中运行,会导致挂起。现有的 overwrite 确认也有同样问题,所以这与项目现有模式一致,暂不阻塞。
3. 错误提示不一致
- 长度超限和字符非法时使用
process.exit(1)退出 - 取消
latest标签时用return并打印❌ Tag creation cancelled(带 ❌ 但用了chalk.yellow)
这与下方已有的 overwrite 取消逻辑风格一致(也是 chalk.yellow + ❌),所以可以接受,但语义上 ❌ 配 yellow 稍显奇怪。
4. 缺少测试
项目目前没有测试文件,这不是本 PR 引入的问题,但新增的验证逻辑(正则匹配、长度限制、保留名检查)非常适合添加单元测试。建议后续补充。
5. 小建议
- 正则可以提取为常量,方便复用和维护:
const TAG_NAME_PATTERN = /^[a-zA-Z0-9._-]+$/;
- 保留名可以用数组管理,便于扩展:
const RESERVED_TAGS = ['latest'];
总结
代码质量良好,验证逻辑合理,与项目现有风格一致。主要建议是补充 issue 中提到的其他保留名和 .. 场景的校验。非阻塞性问题,可以合并后再迭代。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #19
Summary
latest