Copilot commented on code in PR #3624:
URL: https://github.com/apache/avro/pull/3624#discussion_r2688532461
##########
lang/php/lib/Schema/AvroField.php:
##########
@@ -131,13 +184,21 @@ public function toAvro(): string|array
$avro[self::ORDER_ATTR] = $this->order;
}
+ if (!is_null($this->aliases)) {
+ $avro[AvroSchema::ALIASES_ATTR] = $this->aliases;
+ }
+
+ if (!is_null($this->doc)) {
+ $avro[AvroSchema::DOC_ATTR] = $this->doc;
+ }
+
return $avro;
}
/**
* @returns string the name of this field
*/
- public function name()
+ public function name(): ?string
Review Comment:
The return type of the `name()` method is declared as `?string`, but the
property `$name` is actually non-nullable (the constructor accepts `string
$name`). The return type should be `string` instead of `?string` to accurately
reflect that this method always returns a non-null string.
```suggestion
public function name(): string
```
##########
lang/php/lib/Schema/AvroSchema.php:
##########
@@ -544,6 +543,17 @@ public static function hasValidAliases($aliases): void
}
}
+ public static function hasValidDoc(mixed $doc): void
+ {
+ if (is_string($doc) || is_null($doc)) {
+ return;
+ }
+
+ throw new AvroSchemaParseException(
+ 'Invalid doc value. Must be a string or empty.'
Review Comment:
The error message "Must be a string or empty" is misleading. The doc
attribute can be a string or null (absent), but "empty" typically refers to an
empty string "", which is still a valid string. The message should say "Must be
a string or null" to be more accurate.
```suggestion
'Invalid doc value. Must be a string or null.'
```
##########
lang/php/lib/Schema/AvroRecordSchema.php:
##########
@@ -62,62 +62,30 @@ public function __construct(
* @throws AvroSchemaParseException
*/
public static function parseFields(
- array $field_data,
+ array $fieldsDefinitions,
?string $default_namespace,
- ?AvroNamedSchemata $schemata = null
+ AvroNamedSchemata $schemata
): array {
$fields = [];
- $field_names = [];
- $alias_names = [];
- foreach ($field_data as $field) {
- $name = $field[AvroField::FIELD_NAME_ATTR] ?? null;
- $type = $field[AvroSchema::TYPE_ATTR] ?? null;
- $order = $field[AvroField::ORDER_ATTR] ?? null;
- $aliases = $field[AvroSchema::ALIASES_ATTR] ?? null;
-
- $default = null;
- $has_default = false;
- if (array_key_exists(AvroField::DEFAULT_ATTR, $field)) {
- $default = $field[AvroField::DEFAULT_ATTR];
- $has_default = true;
- }
+ $fieldNames = [];
+ $aliasNames = [];
+ foreach ($fieldsDefinitions as $fieldsDefinition) {
+ $name = $fieldsDefinition[AvroField::FIELD_NAME_ATTR] ?? null;
- if (in_array($name, $field_names)) {
+ if (in_array($name, $fieldNames)) {
throw new AvroSchemaParseException(
sprintf("Field name %s is already in use", $name)
);
}
- $is_schema_from_schemata = false;
- $field_schema = null;
- if (
- is_string($type)
- && $field_schema = $schemata->schemaByName(
- new AvroName($type, null, $default_namespace)
- )
- ) {
- $is_schema_from_schemata = true;
- } elseif (is_string($type) && self::isPrimitiveType($type)) {
- $field_schema = self::subparse($field, $default_namespace,
$schemata);
- } else {
- $field_schema = self::subparse($type, $default_namespace,
$schemata);
- }
+ $new_field = AvroField::fromFieldDefinition($fieldsDefinition,
$default_namespace, $schemata);
Review Comment:
The variable name should be singular `$fieldDefinition` to match the
singular context (it represents one field definition from the iteration). This
should also be updated on lines 73 and 81 where the variable is used.
--
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]
