mtesseract / nakadi-client

Haskell Client Library for the Nakadi Event Broker
Other
13 stars 9 forks source link

Removed enrichment constraint from streamed type #70

Closed amrhassan closed 6 years ago

amrhassan commented 6 years ago

In effort to address the problem of not being able to stream events of a type other than DataChangeEvents (as described in #68).

mtesseract commented 6 years ago

Hi,

thanks again for the PR, in particular for fixing this bogus specialization to DataChangeEvent a.

But I am not sure about the renaming thing. After some initial concerns I thought it might be good from a self-documenting-code perspective. But, on the other hand, this naming might make code more confusing when the additional enriched fields are not actually used. Say, a user is not interested in the additional fields for some reason (an example might be an "Echo" service for republishing events).

If specially named Received… verions of the event data types exist, then I would expect them to be somehow "required" for receiving events. On the other hand Enriched is a little bit more transparent: These are just like the original versions but with some Nakadi-enriching added. But you can generally replace the Enriched versions with the non-Enriched versions if you don't need the added Nakadi metadata.

Does this make sense?

amrhassan commented 6 years ago

Hey @mtesseract. Naming is hard :smiley:. I have no strong preference regarding the renaming, so I'm switching it back. I've kept the renaming of Metadata to EventMetadata to be the same as in the Nakadi doc though, but I can rename it back if you think against it.

mtesseract commented 6 years ago

Hi,

I think this is looking good! I guess you are right that the MetadataEventMetadata renaming makes the API closer to Nakadi's API. I'm not a big fan of doing such renamings in a minor version bump, but since you are probably the only user of this library, I guess this is fine. :-)

In the future we should probably use some deprecation method for this.

As I wrote, I think there is an export missing. But if you add this, I'm fine with merging.

Thanks!

mtesseract commented 6 years ago

Thanks!