Skip to content

Conversation

@ihaveint
Copy link

Summary

Refactored artie.Message to use proper Go embedding instead of a named field, following Go best practices for composition.

Changes

  • Changed from message kafka.Message field to embedded kafka.Message
  • Removed redundant wrapper methods (GetMessage(), Topic(), Key(), etc.)
  • Updated usage to direct field access (msg.Topic vs msg.Topic())

Benefits

  • More idiomatic Go code
  • Eliminates code duplication
  • Direct access to all kafka.Message fields/methods

Breaking Changes

⚠️ Removes public methods - external users need to switch from method calls to field access (e.g., msg.Topic()msg.Topic). Impact unclear due to unknown external usage.

Changed Message from named field to embedded kafka.Message struct, enabling direct field access and removing redundant wrapper methods.
@ihaveint ihaveint requested a review from a team as a code owner September 25, 2025 19:57
@williamhaw
Copy link
Contributor

williamhaw commented Sep 25, 2025

I have another branch that I'm working on so that we can move from kafka-go to franz-go, what do you think?

d72c977

@Tang8330
Copy link
Contributor

This is just using anonymous structs? We prefer to avoid usage of anonymous structs for key data models as it causes confusion by conflating methods.

@ihaveint
Copy link
Author

ihaveint commented Oct 1, 2025

This is just using anonymous structs? We prefer to avoid usage of anonymous structs for key data models as it causes confusion by conflating methods.

Thanks for the feedback. Just to clarify terminology: this isn’t an anonymous struct, but rather embedding a field anonymously inside the struct. The intent was to make the composition more idiomatic Go and avoid the somewhat confusing situation where we have a type Message that also has a field message and a method GetMessage.

That said, I understand your concern about method conflation. If the team prefers explicit named fields for clarity, I’m happy to adjust the PR accordingly or close it if that’s the consensus.

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