Copilot commented on code in PR #3528:
URL: https://github.com/apache/avro/pull/3528#discussion_r2464957226
##########
lang/php/lib/Schema/AvroNamedSchema.php:
##########
@@ -29,55 +29,32 @@
class AvroNamedSchema extends AvroSchema
{
/**
- * @var AvroName $name
- */
- private $name;
-
- /**
- * @var string documentation string
- */
- private $doc;
- /**
- * @var array
- */
- private $aliases;
-
- /**
- * @param string $type
- * @param AvroName $name
- * @param string $doc documentation string
- * @param AvroNamedSchemata &$schemata
- * @param array $aliases
* @throws AvroSchemaParseException
*/
- public function __construct($type, $name, $doc = null, &$schemata = null,
$aliases = null)
- {
+ public function __construct(
+ string $type,
+ private AvroName $name,
+ private ?string $doc = null,
+ ?AvroNamedSchemata &$schemata = null,
+ private ?array $aliases = null
+ ) {
parent::__construct($type);
- $this->name = $name;
Review Comment:
The validation of $this->aliases has been moved outside the constructor
parameter validation. In the old code, hasValidAliases was called on the
$aliases parameter before assignment. Now it's called on $this->aliases after
assignment via constructor property promotion, but the constructor doesn't
validate $doc anymore despite accepting it. Add validation: `if ($doc &&
!is_string($doc)) { throw new AvroSchemaParseException('Schema doc attribute
must be a string'); }`
```suggestion
if ($this->doc !== null && !is_string($this->doc)) {
throw new AvroSchemaParseException('Schema doc attribute must be
a string');
}
```
##########
lang/php/lib/Schema/AvroNamedSchemata.php:
##########
@@ -28,17 +28,17 @@
*/
class AvroNamedSchemata
{
- /**
- * @var AvroNamedSchema[]
- */
- private $schemata;
-
/**
* @param AvroNamedSchemata[]
Review Comment:
Duplicate @param documentation with conflicting types. Line 32 documents the
constructor parameter as 'AvroNamedSchemata[]' while line 33 documents
$schemata as 'AvroNamedSchema[]'. The parameter $schemata should be documented
as an array of AvroNamedSchema instances, not AvroNamedSchemata.
```suggestion
```
##########
lang/php/build.sh:
##########
@@ -77,7 +86,7 @@ do
;;
*)
- echo "Usage: $0
{interop-data-generate|test-interop|lint|test|dist|clean}"
+ echo "Usage: $0
{interop-data-generate|test-interop|lint|lint-apply|test|dist|clean}"
Review Comment:
Corrected command name from 'lint-apply' to 'lint-fix' to match the actual
case label defined at line 65.
```suggestion
echo "Usage: $0
{interop-data-generate|test-interop|lint|lint-fix|test|dist|clean}"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
