-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat(tests): 添加 arrayable 和 from 方法的测试用例及相关辅助类 #860
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Poem
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/Macros/ArrTest.php (3)
13-14: 建议改用 Composer autoload 代替require_once手动引入直接
require_once __DIR__ . '/Stubs/Common.php';会绕过 Composer 的自动加载,
导致 IDE 静态分析无法追踪类引用,也容易在文件移动/命名空间调整时遗漏更新。可将
tests/Macros/Stubs目录加入autoload-dev.psr-4,然后删除手动require_once。这样:-require_once __DIR__ . '/Stubs/Common.php';改为在
composer.json:"autoload-dev": { "psr-4": { "FriendsOfHyperf\\Tests\\Macros\\": "tests/Macros/" } }执行
composer dump-autoload即可。
这样既保持测试代码整洁,又避免因路径硬编码带来的维护成本。
15-31: 测试用例重复断言较多,可用数据集精简当前对
Arr::arrayable()的正、负用例分别写了 5+ 条assertTrue/False。
考虑使用 Pest 的dataset()或 PHPUnit 的 dataProvider,用循环一次性覆盖,
减少重复代码并提高可读性。例如:dataset('arrayables', [ [[]], [new TestArrayableObject()], ... ]); test('arrayable positive', function ($value) { expect(Arr::arrayable($value))->toBeTrue(); })->with('arrayables');同理可对负用例和
Arr::from()做类似处理。
非功能性改进,但能让测试文件更简洁。
108-115: WeakMap 场景下建议增加 PHP 版本判断
WeakMap仅在 PHP 8.0+ 可用,虽然项目最低版本大概率 ≥8.0,但为了向后兼容或防止意外降级环境运行测试,可在用例前加:if (!class_exists(WeakMap::class)) { $this->markTestSkipped('WeakMap not supported on this PHP version.'); }避免老版本 CI 直接失败。
若项目已锁定 8.1+ 可忽略此建议。tests/Macros/Stubs/Common.php (2)
21-27: 可考虑将桩类声明为final并补全类型提示这些 Stub 仅用于测试,不需要被继承。将其标记为
final能减少误用并提升 opcache 最佳化效果;同时为构造函数参数、属性及返回值增加显式类型,有助于静态分析。示例:
-final class TestArrayableObject implements Arrayable +final class TestArrayableObject implements Arrayable { public function toArray(): array { return ['foo' => 'bar']; } }同理可应用于其他 Stub 类。
42-48:jsonSerialize()返回类型建议显式标注mixed以兼容 PHP 8.1+PHP 8.1 起
JsonSerializable::jsonSerialize(): mixed,
虽然省略类型不会报错,但显式声明更清晰,亦可避免 Psalm/PhpStan 提示。- public function jsonSerialize(): array + public function jsonSerialize(): array如果未来打算让方法可能返回非数组(例如标量),可直接使用
mixed。src/macros/src/ArrMixin.php (2)
29-36: 建议为 arrayable() 方法添加文档注释
arrayable()方法的实现逻辑正确,能有效判断值是否可转为数组。与类中的其他方法相比,该方法缺少文档注释来说明其功能和用途。+/** + * Determine if the given value is arrayable. + */ public function arrayable() { return fn ($value) => is_array($value) || $value instanceof Arrayable || $value instanceof Traversable || $value instanceof Jsonable || $value instanceof JsonSerializable; }
89-102: 建议为 from() 方法添加文档注释并说明 WeakMap 的特殊处理
from()方法实现全面,使用了 PHP 8 的 match 表达式处理多种类型转换,代码简洁高效。有两点建议:
- 添加文档注释说明方法功能
- 解释为什么 WeakMap 使用
iterator_to_array($items, false)与其他 Traversable 不同的处理方式+/** + * Convert the given value to an array. + * + * @param mixed $items + * @return array + * @throws \InvalidArgumentException + */ public function from() { return fn ($items) => match (true) { is_array($items) => $items, $items instanceof Enumerable => $items->all(), $items instanceof Arrayable => $items->toArray(), + // WeakMap 使用对象作为键,不能保存键值,因此使用 false 参数 $items instanceof WeakMap => iterator_to_array($items, false), $items instanceof Traversable => iterator_to_array($items), $items instanceof Jsonable => json_decode($items->toJson(), true), $items instanceof JsonSerializable => (array) $items->jsonSerialize(), is_object($items) => (array) $items, default => throw new InvalidArgumentException('Items cannot be represented by a scalar value.'), }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/macros/output/Hyperf/Collection/Arr.php(2 hunks)src/macros/src/ArrMixin.php(5 hunks)tests/Macros/ArrTest.php(2 hunks)tests/Macros/Stubs/Common.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/macros/output/Hyperf/Collection/Arr.php (1)
src/macros/src/ArrMixin.php (2)
arrayable(29-36)from(89-102)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.2 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.1 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.3 with Swoole 6.0.2
- GitHub Check: Test on PHP 8.3 with Swoole 5.1.7
- GitHub Check: Test on PHP 8.2 with Swoole 5.1.7
🔇 Additional comments (8)
src/macros/output/Hyperf/Collection/Arr.php (2)
56-69:Details
❌ Incorrect review comment
from()同样为空实现,建议与上文一致处理同样出于防御性编程考虑,建议为
from()增加占位实现或抛出异常,避免误用造成“空数组/null”一类难以排查的问题;并在注释中指明真正逻辑来源(ArrMixin::from()动态宏)。
参见上一条建议,可一并处理。
为验证
Arr::from()的真实实现来源及与其他静态方法的一致性,请运行以下脚本:
🏁 Script executed:
#!/bin/bash # 查找 ArrMixin 类及其 from 方法定义 rg -n "class ArrMixin" -C2 . rg -n "function from" -C3 . # 列出 Arr.php 中所有静态方法,实现空方法的模式 rg -n "public static function" -C1 src/macros/output/Hyperf/Collection/Arr.phpLength of output: 13903
无需为
Arr::from()添加占位实现或抛出异常
src/macros/output/Hyperf/Collection/Arr.php中所有静态方法(包括from)均为自动生成的宏声明(空方法),真正的实现逻辑由ArrMixin::from()在运行时动态注入。保持空实现可确保与其他方法保持一致,并由宏机制接管方法调用,无需额外抛出异常或实现占位。
- 文件:
src/macros/output/Hyperf/Collection/Arr.php- 方法:
public static function from($items) { }Likely an incorrect or invalid review comment.
19-27:Details
✅ Verification successful
静态桩方法缺少任何实现,可能导致运行时误调用隐患
目前
arrayable()只有签名,没有函数体。一旦该桩文件被实际加载(例如误被 autoload 进生产环境),调用Arr::arrayable()会得到null,从而破坏业务逻辑或导致类型错误。
如果此文件仅用于 IDE-stub/静态分析而绝不会被执行,建议:
- 在文件或目录层面通过
composer.json的exclude-from-classmap明确排除,确保运行时永远不会加载到。- 在方法体中显式抛出异常或加入注释说明其纯粹的 stub 性质,以便后续维护者快速理解意图,减少踩坑概率。示例 diff:
public static function arrayable($value) { - } + // This is an analysis-only stub. Real logic is provided at runtime by ArrMixin. + throw new \LogicException('Stub – should never be executed.'); + }
🏁 Script executed:
#!/bin/bash # List the stub file content (first 200 lines) if [ -f src/macros/output/Hyperf/Collection/Arr.php ]; then echo "---- src/macros/output/Hyperf/Collection/Arr.php ----" sed -n '1,200p' src/macros/output/Hyperf/Collection/Arr.php else echo "File src/macros/output/Hyperf/Collection/Arr.php not found." fi # Show composer.json autoload and autoload-dev sections echo "---- composer.json autoload sections ----" grep -R -n "\"autoload" -n composer.json -A 10 # Check for exclude-from-classmap usage echo "---- composer.json exclude-from-classmap ----" grep -R -n "exclude-from-classmap" composer.json || echo "No exclude-from-classmap found."Length of output: 4201
确认:IDE Stub 文件不在运行时 Autoload 范围,无需改动
经检查,
src/macros/output/Hyperf/Collection/Arr.php所在目录并未在composer.json的 PSR-4 或 classmap autoload 中声明,因此这些空方法仅用于 IDE 提示/静态分析,不会在生产环境被加载或调用。• 无需在
composer.json中额外添加exclude-from-classmap。
• 如有必要,可在文件注释中说明仅为 Stub,或在项目文档里集中说明此目录用途,避免维护者误解。
• 建议保持当前空方法实现,不做抛错处理,以免影响静态分析工具对方法签名的识别。src/macros/src/ArrMixin.php (6)
16-22: 新导入的依赖项与添加的功能匹配添加了多个关于可数组化接口的导入,这些都是为了支持新增的
arrayable()和from()方法所需的,导入的依赖项选择恰当。
38-39: 方法从静态改为实例方法符合 Mixin 模式将之前的静态方法改为实例方法,符合该类作为 Mixin 的设计模式,这是一个良好的变更。
56-57: 方法从静态改为实例方法符合 Mixin 模式将之前的静态方法改为实例方法,符合该类作为 Mixin 的设计模式,这是一个良好的变更。
74-75: 方法从静态改为实例方法符合 Mixin 模式将之前的静态方法改为实例方法,符合该类作为 Mixin 的设计模式,这是一个良好的变更。
107-108: 方法从静态改为实例方法符合 Mixin 模式将之前的静态方法改为实例方法,符合该类作为 Mixin 的设计模式,这是一个良好的变更。
125-126: 方法从静态改为实例方法符合 Mixin 模式将之前的静态方法改为实例方法,符合该类作为 Mixin 的设计模式,这是一个良好的变更。
Summary by CodeRabbit
新功能
测试