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

Add missing MkProducer argument, force compiler errors when they're missing #585

Merged
merged 4 commits into from
Apr 9, 2021

Conversation

bplommer
Copy link
Member

@bplommer bplommer commented Apr 8, 2021

I'm not 100% happy with the added boilerplate, or with it being needed, but I don't have a better idea. Any thoughts?

@bplommer bplommer requested review from vlovgr and LMnet April 8, 2021 14:17
@bplommer
Copy link
Member Author

bplommer commented Apr 8, 2021

Exciting update: it doesn't compile with Scala 2.12 anyway. I guess I'll just leave the ambiguous implicits stuff out, but I'm still not happy with that.

@bplommer
Copy link
Member Author

bplommer commented Apr 8, 2021

Exciting update no. 2: It now compiles in Scala 2.12, courtesy of added ugliness.

@LMnet
Copy link
Member

LMnet commented Apr 9, 2021

Could you please explain why do we need this? As far as I understand, without mkAmbig1 and mkAmbig2 default instance of the Mk??? instance will be used (which is derived from the Sync). And what is the problem with that?

@bplommer
Copy link
Member Author

bplommer commented Apr 9, 2021

Could you please explain why do we need this?

It's not strictly needed, but it's an extra safeguard against us writing factory methods that don't require an Mk argument.

As far as I understand, without mkAmbig1 and mkAmbig2 default instance of the Mk??? instance will be used (which is derived from the Sync). And what is the problem with that?

There's no problem with the default instance derived from Sync being used, but that derivation needs to happen at the call site in user code so that the user can override the default behaviour if needed.

@LMnet
Copy link
Member

LMnet commented Apr 9, 2021

Oh, so that's for library maintainers mostly, not for users. Ok, I got it.

@bplommer
Copy link
Member Author

bplommer commented Apr 9, 2021

This is intended to catch bugs like the one this PR fixes on lines 642-63 of KafkaProducerConnection.scala, where the stream method delegates to the resource method. Rather than stream taking a MkProducer argument it was autoderiving an instance based on Sync, which would prevent use of a custom instance by user code.

@bplommer
Copy link
Member Author

bplommer commented Apr 9, 2021

Oh, so that's for library maintainers mostly, not for users. Ok, I got it.

👍 didn't see your reply before my second comment

@bplommer bplommer merged commit f269875 into series/2.x Apr 9, 2021
@bplommer bplommer deleted the missing-mk branch April 9, 2021 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants