feat: support tel type validator #21
Conversation
Walkthrough添加对电话号码类型 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @guoyunhe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new tel validator, which is a useful addition. The changes correctly integrate the new type across the README.md, src/interface.ts, src/messages.ts, src/util.ts, and src/validator/index.ts files. A new test file tests/tel.spec.ts has been added with several test cases covering various phone number formats and the required rule. However, there is a potential issue with the arbitrary length limit imposed on phone numbers, which could lead to valid international numbers being rejected.
There was a problem hiding this comment.
Pull request overview
This PR adds support for a new tel type validator to handle phone number validation. The implementation follows the existing pattern used by other type validators like email and url.
Key Changes:
- Adds a new telephone number validator with regex-based validation supporting international formats
- Implements validation for various phone number formats including country codes, brackets, spaces, dots, and dashes
- Integrates the new validator across all necessary files (validator registry, type system, messages, and documentation)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tel.spec.ts | Adds test suite covering various valid phone number formats (Chinese and US styles) |
| src/validator/index.ts | Registers the new tel validator in the validator exports |
| src/util.ts | Adds tel to the list of native string types for empty value handling |
| src/rule/type.ts | Implements the core validation logic with regex pattern and 32-character limit |
| src/messages.ts | Adds error message template for invalid telephone numbers |
| src/interface.ts | Adds tel to the RuleType union and ValidateMessages interface |
| README.md | Documents the new tel type in the usage guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import Schema from '../src'; | ||
|
|
||
| describe('tel', () => { | ||
| it('works for empty string', done => { | ||
| new Schema({ | ||
| v: { | ||
| type: 'tel', | ||
| }, | ||
| }).validate( | ||
| { | ||
| v: '', | ||
| }, | ||
| errors => { | ||
| expect(errors).toBe(null); | ||
| done(); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| it('works for china mobile phone number', done => { | ||
| new Schema({ | ||
| v: { | ||
| type: 'tel', | ||
| }, | ||
| }).validate( | ||
| { | ||
| v: '13156451303', | ||
| }, | ||
| errors => { | ||
| expect(errors).toBe(null); | ||
| done(); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| it('works for china mobile phone number with country code', done => { | ||
| new Schema({ | ||
| v: { | ||
| type: 'tel', | ||
| }, | ||
| }).validate( | ||
| { | ||
| v: '+8613156451303', | ||
| }, | ||
| errors => { | ||
| expect(errors).toBe(null); | ||
| done(); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| it('works for china mobile phone number with spaces', done => { | ||
| new Schema({ | ||
| v: { | ||
| type: 'tel', | ||
| }, | ||
| }).validate( | ||
| { | ||
| v: '+86 131 5645 1303', | ||
| }, | ||
| errors => { | ||
| expect(errors).toBe(null); | ||
| done(); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| it('works for us phone number with brackets, dashes, and spaces', done => { | ||
| new Schema({ | ||
| v: { | ||
| type: 'tel', | ||
| }, | ||
| }).validate( | ||
| { | ||
| v: '(123) 456-7890', | ||
| }, | ||
| errors => { | ||
| expect(errors).toBe(null); | ||
| done(); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| it('works for required empty string', done => { | ||
| new Schema({ | ||
| v: { | ||
| type: 'tel', | ||
| required: true, | ||
| }, | ||
| }).validate( | ||
| { | ||
| v: '', | ||
| }, | ||
| errors => { | ||
| expect(errors.length).toBe(1); | ||
| expect(errors[0].message).toBe('v is required'); | ||
| done(); | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| }); |
There was a problem hiding this comment.
The test suite lacks negative test cases to verify that invalid phone numbers are properly rejected. Consider adding test cases for invalid inputs such as:
- Invalid formats like "abc123" or "123abc"
- Single digit inputs like "1" or "12"
- Only special characters like "---" or " "
- Too long or too short phone numbers
- Invalid character combinations
Other type validators in the codebase (e.g., url.spec.ts) include negative test cases to ensure invalid inputs are rejected. This helps ensure the regex pattern works correctly and catches edge cases.
src/rule/type.ts
Outdated
| * @see https://regexr.com/3c53v | ||
| * @see https://ihateregex.io/expr/phone/ | ||
| */ | ||
| tel: /^(\+[0-9]{1,3}[-\s\.]?)?(\([0-9]{1,4}\))?[-\s\.0-9]+$/, |
There was a problem hiding this comment.
The phone number regex pattern is too permissive and will accept invalid inputs. The pattern uses [-\s\.0-9]+$ as the main number part, which allows:
- Single digit numbers like "1" or "12"
- Only special characters like "---" or "..." or " "
- Any combination of digits, spaces, dots, and dashes without requiring actual digits
Consider strengthening the regex to require:
- A minimum number of actual digits (e.g., at least 7-10 digits depending on requirements)
- At least one digit in the main number part to prevent inputs like "---" from being valid
- More specific formatting rules if you want to enforce stricter validation
A more robust pattern might ensure that there are actual digits present and not just separators.
| tel: /^(\+[0-9]{1,3}[-\s\.]?)?(\([0-9]{1,4}\))?[-\s\.0-9]+$/, | |
| tel: /^(\+[0-9]{1,3}[-\s.]?)?(?:\([0-9]{1,4}\))?(?:[-\s.]?\d){7,}$/, |
| return typeof value === 'string' && value.length <= 320 && !!value.match(pattern.email); | ||
| }, | ||
| tel(value: Value) { | ||
| return typeof value === 'string' && value.length <= 32 && !!value.match(pattern.tel); |
There was a problem hiding this comment.
The 32 character limit for phone numbers might be too restrictive for some international phone number formats that include country codes, area codes, and extensions. Consider whether this limit is appropriate for your use case. For reference, the E.164 standard allows up to 15 digits, but with formatting characters (spaces, dashes, parentheses, plus sign), phone numbers could legitimately exceed 32 characters, especially with extensions.
| return typeof value === 'string' && value.length <= 32 && !!value.match(pattern.tel); | |
| return typeof value === 'string' && value.length <= 64 && !!value.match(pattern.tel); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @src/rule/type.ts:
- Around line 15-20: The tel regex on the tel property allows only-separator
strings to match because the final character class [-\s\.0-9]+ can be satisfied
without digits; update the tel pattern to require at least one digit (e.g.,
ensure a lookahead like (?=.*\d) or require \d somewhere in the main pattern)
or, if you prefer not to change the regex, add a numeric check in the tel
validation function (tel()): after the regex test, assert that the value
contains at least one digit (/\d/). Target the tel property in src/rule/type.ts
and/or the tel() validator function to implement one of these fixes.
🧹 Nitpick comments (2)
README.md (1)
186-186: 建议：补充 tel 类型的格式说明文档条目的格式与其他类型保持一致。不过，考虑到电话号码格式的多样性，建议添加更详细的说明，例如支持的格式示例。
🔎 建议的文档改进
-- `tel`: Must be of type `tel`. +- `tel`: Must be of type `tel`. Supports various phone number formats with optional country code (e.g., `+86`), area code in parentheses (e.g., `(123)`), and separators (spaces, dashes, dots).或者，可以在文档中添加一个单独的小节来详细说明 tel 类型支持的格式，类似于其他复杂类型的处理方式。
tests/tel.spec.ts (1)
20-82: 应补充无效电话号码格式的测试用例以完善覆盖范围。当前测试仅验证了有效的电话号码格式。建议添加测试用例以确保验证器正确拒绝以下无效输入：
- 包含字母或特殊符号的输入（如
abc123）- 仅包含分隔符的输入（如
---或）- 超短或无效的格式组合
建议的补充测试用例
it('should reject phone number with letters', done => { new Schema({ v: { type: 'tel', }, }).validate( { v: 'abc123', }, errors => { expect(errors.length).toBe(1); done(); }, ); }); it('should reject phone number with only separators', done => { new Schema({ v: { type: 'tel', }, }).validate( { v: '---', }, errors => { expect(errors.length).toBe(1); done(); }, ); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mdsrc/interface.tssrc/messages.tssrc/rule/type.tssrc/util.tssrc/validator/index.tstests/tel.spec.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/rule/type.ts (1)
src/interface.ts (1)
Value(182-182)
🔇 Additional comments (9)
src/rule/type.ts (2)
83-96: 实现正确！
'tel'已正确添加到自定义类型数组中，这样rule.type === 'tel'就会通过types映射进行验证。实现与现有类型验证器的集成方式保持一致。
67-69: 补充测试覆盖以确保边界情况完整函数结构正确，现有测试已覆盖空字符串、国际格式和各种分隔符场景。但仍需补充以下测试用例：
- 32 字符长度限制的边界测试
- 超出 32 字符的字符串（应返回错误）
- 仅分隔符的字符串（如
"---"或" "，当前正则可能误判为有效）- 无数字但包含有效分隔符的字符串
建议扩展
tests/tel.spec.ts的测试覆盖范围以验证这些边界场景。src/messages.ts (1)
26-26: 实现正确！错误消息
'%s is not a valid %s'与其他类型验证消息（如 email、url、hex）的格式保持一致。格式化后的消息将清晰地指示字段验证失败。src/validator/index.ts (1)
33-33: 实现正确！将
tel: type添加到导出对象中的方式与现有的url、hex、type: 'tel'了。src/util.ts (1)
87-97: 实现正确！将
'tel'添加到isNativeStringType检查中是正确的做法。这确保了：
- tel 类型的值被视为原生字符串类型（与 email、url 等一致）
- 空字符串在非必填时会被
isEmptyValue正确识别为空值- 验证流程与其他字符串类型保持一致
src/interface.ts (2)
18-18: 变更看起来不错！将 'tel' 添加到 RuleType 联合类型中，位置合理，与其他通信相关类型（如 'email'、'url'）保持一致。
150-150: 类型定义正确！为 ValidateMessages.types 添加 tel 消息的类型定义，签名与其他类型验证器保持一致。
tests/tel.spec.ts (2)
4-18: 测试逻辑正确！空字符串在非必填时通过验证，这是标准的验证器行为。
84-100: 必填验证测试正确！正确测试了当字段为必填时，空字符串应产生验证错误。
|
@gemini-code-assist 请重新 CR，使用中文评论。主要看看正则表达式是否有问题，是否需要优化。 |
There was a problem hiding this comment.
Code Review
This pull request introduces a new tel validator for phone numbers. The changes are well-structured and cover type definitions, validation logic, and documentation. However, I've found a couple of areas for improvement. The regular expression for phone number validation is too permissive and can accept invalid inputs like strings containing only spaces or dashes. I've suggested a fix to make it more robust. Additionally, the new test suite only includes positive test cases for valid phone numbers. I recommend adding negative test cases to ensure the validator correctly rejects invalid formats. Overall, this is a great addition, and with these changes, it will be even more reliable.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21 +/- ##
==========================================
+ Coverage 82.33% 82.41% +0.08%
==========================================
Files 25 25
Lines 651 654 +3
Branches 232 234 +2
==========================================
+ Hits 536 539 +3
Misses 114 114
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Support various typical phone number formats:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.