Skip to content

Conversation

@Knerio
Copy link
Member

@Knerio Knerio commented Nov 27, 2025

No description provided.

@Knerio Knerio force-pushed the 162-genericmapperto_h-is-implemented-incorrectly branch from a3f7e18 to 8736b99 Compare November 27, 2025 21:07
@Taucher2003 Taucher2003 merged commit 33954fc into main Nov 27, 2025
3 checks passed
@Taucher2003 Taucher2003 deleted the 162-genericmapperto_h-is-implemented-incorrectly branch November 27, 2025 21:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the serialization of the source field in the GenericMapper class by correctly mapping over an array of DataTypeIdentifier objects. According to the protobuf definition (repeated DataTypeIdentifier source = 1), source is an array, not a single object.

  • Changed self.source.to_h to self.source.map(&:to_h) to properly serialize the array

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
target: self.target,
source: self.source.to_h,
source: self.source.map(&:to_h),
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix to use self.source.map(&:to_h) is correct based on the protobuf definition (line 155: repeated DataTypeIdentifier source = 1), but the corresponding from_hash method on line 248 is inconsistent. Line 248 treats source as a single object: self.source = DataTypeIdentifier.from_hash(config.fetch(:source)), but it should map over an array like:

self.source = config.fetch(:source).map { |s| DataTypeIdentifier.from_hash(s) }

This inconsistency will cause serialization to work but deserialization to fail.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants