Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Attributes interface #118

Closed
bsideup opened this issue Apr 20, 2020 · 1 comment
Closed

Remove Attributes interface #118

bsideup opened this issue Apr 20, 2020 · 1 comment

Comments

@bsideup
Copy link
Contributor

bsideup commented Apr 20, 2020

In the current version of SDK, one has to deal with generic signature of CloudEvent and an unnecessary allocation of the AttributesImpl object.

While working on CloudEvents for Liiklus, I had to implement our own CloudEvent type that implements both CloudEvent and Attributes to reduce the allocations (see the link)

Since (according to the spec) there is always an 1:1 mapping of an event and its attributes, I suggest to remove the interface and make the fields part of the CloudEvent interface.

To make the change less impactful and support a migration path, I suggest we do it in two steps (that should be released separately):

  1. Make CloudEvent implementations implement both CloudEvent and Attributes and return this from getAttributes()
  2. Make CloudEvent extend from Attributes and turn getAttributes() into a default method that returns this and deprecate getAttributes() and Attributes
  3. Remove Attributes and ``getAttributes()and move the fields to theCloudEvent` interface

If the community agrees, I can work on all 3 PRs.

This was referenced Apr 20, 2020
@slinkydeveloper
Copy link
Member

Now that the generics problem doesn't exist anymore, I think this discussion really brings us down to the "inheritance vs composition" debate, which doesn't have a real winner here. I think we should keep as is for several reasons:

  • Easier to implement conversions back/forth different spec versions
  • Easier to implement Message interfaces
  • Less burden to have a new spec version
  • Only one CloudEvent interface impl that already handles all the CloudEvent various methods
  • A user can handle Attributes separately to the Event and eventually cast down the specific AttributesImpl to handle specversion specific informations, and we can still hide the details of CloudEventImpl
  • Most important IMO: consistency with other SDKs

From a usability POV, I think CloudEvent interface can extend Attributes and delegate the methods to AttributesImpl.

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

No branches or pull requests

2 participants