Wrap callbacks in newtype to make wrong usage harder

[phile314]

I was at first struggling to use the callbacks correctly. Granted, this was mostly my fault as I looked at the wrong hackage version of hw-client which did not yet have the examples in the doc on how to use the callbacks. But anyway, I find having the ... -> k -> IO () in the type signature a bit confusing. I was basically doing:

producer <- newProducer ...
statsCallback cb producer

instead of using setCallback and the producer properties.

I think just wrapping the callbacks in a newtype makes it hard (impossible?) to make this mistake and improves readability. What do you think?

PS: Backward-compatiblity is only affected if people gave explicity type signatures for any variable containing the callback. If one just directly passes the callback to setCallback no change is required. E.g.:

  -- Global producer properties
  producerProps :: ProducerProperties
  producerProps = brokersList [BrokerAddress "localhost:9092"]
               <> sendTimeout (Timeout 10000)
               <> setCallback (deliveryCallback print)
               <> logLevel KafkaLogDebug

stil compiles.

[sir4ur0n]

This is a great idea, thank you for this PR!

I put some comments where I think we can improve the API.
Other than that, I am 100% onboard with these changes (though I'm not the project owner so I can't merge it 😄 )

[sir4ur0n]

Same remark

[sir4ur0n]

Any reason why you also renamed this variable?

[phile314]

The issue here is that because the KafkaConf argument moves into the newtype, the variable is now bound in the lambda instead of the function definition:

   Callback $ \kc@(KafkaConf con _ _) -> rdKafkaConfSetRebalanceCb con (realCb kc)

This means kc is no longer available in the where-clause, hence I added it as an extra argument to realCb. The other obvious alternative would be to move realCb into a let, but I am not sure that is better.

[sir4ur0n]

My apologies, I misread, I thought k was renamed to kc but it's another argument

[sir4ur0n]

Looks good to me.

@AlexeyRaga what do you think? Keep in mind this is a breaking change, so this should probably be documented somewhere (like a changelog) to help users migrate

[AlexeyRaga]

Thanks guys, I will review it soon!

[AlexeyRaga]

Looks good to me! Thanks @phile314, I like it! 👍

If you could rebase (I merged a couple of your other PRs) and resolve conflicts then we can hit the green button and make a release!

[phile314]

Rebased on current master.

And thanks already for making a release with these changes, it is greatly appreciated 👍