Copilot commented on code in PR #3317:
URL: https://github.com/apache/avro/pull/3317#discussion_r2454655431
##########
lang/ruby/lib/avro/schema_validator.rb:
##########
@@ -198,23 +198,38 @@ def validate_union(expected_schema, datum, path, result,
options)
compatible_type = first_compatible_type(datum, expected_schema, path,
failures, options)
return unless compatible_type.nil?
- complex_type_failed = failures.detect { |r|
COMPLEX_TYPES.include?(r[:type]) }
- if complex_type_failed
- complex_type_failed[:result].errors.each { |error| result << error }
- else
- types = expected_schema.schemas.map { |s| "'#{s.type_sym}'"
}.join(', ')
- result.add_error(path, "expected union of [#{types}], got
#{actual_value_message(datum)}")
+
+ failed_complex_types = failures.select { |r|
COMPLEX_TYPES.include?(r[:type]) }
+ complex_type_errors = []
+ failed_complex_types.each do |failed_complex_type|
+ error_msg = failed_complex_type[:result].errors.map do |error|
+ error
+ end.join("; ")
+ schema_name_prefix = "#{failed_complex_type[:schema_name]}: " if
failed_complex_type[:schema_name]
+ complex_type_errors << "#{schema_name_prefix}#{error_msg}"
end
+
+ types = expected_schema.schemas.map do |s|
+ s.respond_to?(:name) ? "#{s.name} ('#{s.type}')" : "'#{s.type}'"
+ end.join(', ')
+ type_mismatches = %Q{\nUnion type specific
errors:\n#{complex_type_errors.join("\n")}} if complex_type_errors.any?
+ result.add_error(path, "expected union of [#{types}], got
#{actual_value_message(datum)}#{type_mismatches}")
end
def first_compatible_type(datum, expected_schema, path, failures,
options = {})
expected_schema.schemas.find do |schema|
# Avoid expensive validation if we're just validating a nil
- next datum.nil? if schema.type_sym == :null
+ if schema.type_sym == :null
+ next datum.nil?
+ end
Review Comment:
[nitpick] The refactoring from inline conditional to multi-line if/end adds
unnecessary verbosity for a simple early-exit condition. The original one-line
format `next datum.nil? if schema.type_sym == :null` is more idiomatic Ruby.
```suggestion
next datum.nil? if schema.type_sym == :null
```
##########
lang/ruby/lib/avro/schema_validator.rb:
##########
@@ -198,23 +198,38 @@ def validate_union(expected_schema, datum, path, result,
options)
compatible_type = first_compatible_type(datum, expected_schema, path,
failures, options)
return unless compatible_type.nil?
- complex_type_failed = failures.detect { |r|
COMPLEX_TYPES.include?(r[:type]) }
- if complex_type_failed
- complex_type_failed[:result].errors.each { |error| result << error }
- else
- types = expected_schema.schemas.map { |s| "'#{s.type_sym}'"
}.join(', ')
- result.add_error(path, "expected union of [#{types}], got
#{actual_value_message(datum)}")
+
+ failed_complex_types = failures.select { |r|
COMPLEX_TYPES.include?(r[:type]) }
+ complex_type_errors = []
+ failed_complex_types.each do |failed_complex_type|
+ error_msg = failed_complex_type[:result].errors.map do |error|
+ error
+ end.join("; ")
Review Comment:
The map block is redundant as it simply returns each error unchanged.
Replace with `failed_complex_type[:result].errors.join('; ')` to simplify the
code.
```suggestion
error_msg = failed_complex_type[:result].errors.join("; ")
```
##########
lang/ruby/lib/avro/schema_validator.rb:
##########
@@ -198,23 +198,38 @@ def validate_union(expected_schema, datum, path, result,
options)
compatible_type = first_compatible_type(datum, expected_schema, path,
failures, options)
return unless compatible_type.nil?
- complex_type_failed = failures.detect { |r|
COMPLEX_TYPES.include?(r[:type]) }
- if complex_type_failed
- complex_type_failed[:result].errors.each { |error| result << error }
- else
- types = expected_schema.schemas.map { |s| "'#{s.type_sym}'"
}.join(', ')
- result.add_error(path, "expected union of [#{types}], got
#{actual_value_message(datum)}")
+
+ failed_complex_types = failures.select { |r|
COMPLEX_TYPES.include?(r[:type]) }
+ complex_type_errors = []
+ failed_complex_types.each do |failed_complex_type|
+ error_msg = failed_complex_type[:result].errors.map do |error|
+ error
+ end.join("; ")
+ schema_name_prefix = "#{failed_complex_type[:schema_name]}: " if
failed_complex_type[:schema_name]
+ complex_type_errors << "#{schema_name_prefix}#{error_msg}"
end
+
+ types = expected_schema.schemas.map do |s|
+ s.respond_to?(:name) ? "#{s.name} ('#{s.type}')" : "'#{s.type}'"
+ end.join(', ')
+ type_mismatches = %Q{\nUnion type specific
errors:\n#{complex_type_errors.join("\n")}} if complex_type_errors.any?
+ result.add_error(path, "expected union of [#{types}], got
#{actual_value_message(datum)}#{type_mismatches}")
end
def first_compatible_type(datum, expected_schema, path, failures,
options = {})
expected_schema.schemas.find do |schema|
# Avoid expensive validation if we're just validating a nil
- next datum.nil? if schema.type_sym == :null
+ if schema.type_sym == :null
+ next datum.nil?
+ end
result = Result.new
validate_recursive(schema, datum, path, result, options)
- failures << { type: schema.type_sym, result: result } if
result.failure?
+ if result.failure?
+ failure = { type: schema.type_sym, result: result }
+ failure[:schema_name] = schema.name if schema.respond_to?(:name)
Review Comment:
[nitpick] The refactoring from inline conditional to multi-line if/end
reduces readability for what was a simple one-line append operation. Consider
keeping the original inline style or combining the hash construction into a
single expression.
```suggestion
failure = { type: schema.type_sym, result: result,
**(schema.respond_to?(:name) ? { schema_name: schema.name } : {}) }
```
--
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]
