martin-g commented on code in PR #3624:
URL: https://github.com/apache/avro/pull/3624#discussion_r2688000660
##########
lang/php/test/SchemaTest.php:
##########
@@ -232,6 +232,27 @@ public function test_validate_repeated_aliases(): void
);
}
+ public function test_doc_attribute_on_primitive_fields(): void
+ {
+ $schemaJson = <<<JSON
+ {
+ "type": "record",
+ "name": "fruits",
+ "fields": [
+ {
+ "name": "banana",
+ "type": "string",
+ "doc": "This is a banana"
Review Comment:
Please add a negative test with a non-string value:
```json
"doc": 123
```
To see how it fails.
##########
lang/php/lib/Schema/AvroField.php:
##########
Review Comment:
```suggestion
public function getDoc(): ?string
{
return $this->doc;
}
public function hasDoc(): bool
{
return null !== $this->doc;
}
```
Do we need getter+has methods like the other fields ?
##########
lang/php/test/SchemaTest.php:
##########
@@ -232,6 +232,27 @@ public function test_validate_repeated_aliases(): void
);
}
+ public function test_doc_attribute_on_primitive_fields(): void
+ {
+ $schemaJson = <<<JSON
+ {
+ "type": "record",
+ "name": "fruits",
+ "fields": [
+ {
+ "name": "banana",
+ "type": "string",
+ "doc": "This is a banana"
+ }
+ ]
+ }
+ JSON;
+
+ $schema = AvroSchema::parse($schemaJson);
+
+ self::assertEquals($schemaJson, json_encode($schema->toAvro(), flags:
JSON_PRETTY_PRINT));
Review Comment:
Wouldn't it be more robust to compare the decoded JSON instead of comparing
strings ?
```suggestion
self::assertEquals(json_decode($schemaJson, true),
$schema->toAvro());
```
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -131,13 +135,17 @@ public function toAvro(): string|array
$avro[self::ORDER_ATTR] = $this->order;
}
Review Comment:
Is it intentional that `aliases` are not used here ?
```suggestion
if (!is_null($this->aliases)) {
$avro[AvroSchema::ALIASES_ATTR] = $this->aliases;
}
```
--
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]
