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 producer convenience methods #632

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Conversation

keirlawson
Copy link
Contributor

Per suggestions in #446

Tests are def a bit anaemic, wasn't sure what the right balance between exhaustivity and maintainability was.

Didn't include a pipe method as suggested in the original issue as was not clear what the semantics should be (flatten the F[F[...]]?), but happy to add if clarified.

@bplommer
Copy link
Member

This looks great! Just one thing - adding these as instance methods is going to break binary compatibility, so I'm thinking it would be better to have them as extension methods implemented in terms of produce. Would that work?

@keirlawson
Copy link
Contributor Author

Should be do-able, where would you place said extension methods?

@bplommer
Copy link
Member

Should be do-able, where would you place said extension methods?

I'd say an implicit class in companion object so they're always available. It'll need some kind of typeclass available at the usage site (I think Applicative or Monad?) but that's ok.

def produceOne_(record: ProducerRecord[K,V]): F[F[RecordMetadata]] = {
produceOne(record, ()).map(_.flatMap { res =>
val metadata = res.records.head.map(_._2)
MonadError[F,Throwable].fromOption(metadata, new IllegalStateException("Exactly one record metadata expected"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only reason we need MonadThrow? If so I'd be inclined to just throw an exception here, since this should never happen.

@bplommer
Copy link
Member

Didn't include a pipe method as suggested in the original issue as was not clear what the semantics should be (flatten the F[F[...]]?), but happy to add if clarified.

Sorry, forgot about this - I was thinking it would be the same as the existing pipe but as an extension method:

def pipe[F[_]: Concurrent, K, V, P](

That requires ProducerSettings though, which would be weird on an extension method, so let's leave it for now 🤔

@bplommer
Copy link
Member

One other thing I forgot - can you make the ops class extend AnyVal?

@keirlawson
Copy link
Contributor Author

One other thing I forgot - can you make the ops class extend AnyVal?

Doing so I get value class needs to have exactly one val parameter, presumably because the context bound desugars to an implicit parameter?

@bplommer
Copy link
Member

bplommer commented Jun 15, 2021

iirc the producer parameter should be a private val.

@keirlawson
Copy link
Contributor Author

iirc the producer parameter should be a private val.

I get the same error with that

@LMnet
Copy link
Member

LMnet commented Jun 16, 2021

Doing so I get value class needs to have exactly one val parameter, presumably because the context bound desugars to an implicit parameter?

Yes, to use extends AnyVal you have to move Functor requirement to the methods, as an implicit parameter.

Also, I would add tests for each method. These tests could be dumb, but each new method should be called in tests. Because these multiple overrides could lead to runtime issues with erasure, especially after refactorings.

@bplommer
Copy link
Member

Doing so I get value class needs to have exactly one val parameter, presumably because the context bound desugars to an implicit parameter?

Yes, to use extends AnyVal you have to move Functor requirement to the methods, as an implicit parameter.

Right, yeah. On second thought, maybe these should just be final instance methods implemented in the trait - what do you guys think?

@keirlawson
Copy link
Contributor Author

Doing so I get value class needs to have exactly one val parameter, presumably because the context bound desugars to an implicit parameter?

Yes, to use extends AnyVal you have to move Functor requirement to the methods, as an implicit parameter.

Right, yeah. On second thought, maybe these should just be final instance methods implemented in the trait - what do you guys think?

Personally not strong opinions, happy to do whichever folk reckon is best :)

@bplommer
Copy link
Member

Also, I would add tests for each method. These tests could be dumb, but each new method should be called in tests. Because these multiple overrides could lead to runtime issues with erasure, especially after refactorings.

I think erasure issues should show up at compile time? But yes, it would be good to at least see that code using these compiles.

@LMnet
Copy link
Member

LMnet commented Jun 16, 2021

On second thought, maybe these should just be final instance methods implemented in the trait - what do you guys think?

I'm on the Ops side. I think basic interface should be as simple as possible. And all custom stuff on top of it could be done in the separate Ops trait.

@bplommer
Copy link
Member

bplommer commented Jun 16, 2021

I'm on the Ops side. I think basic interface should be as simple as possible. And all custom stuff on top of it could be done in the separate Ops trait.

I don't have a strong opinion so let's go with that 🙂

Copy link
Member

@bplommer bplommer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks!

@bplommer bplommer requested a review from LMnet June 16, 2021 11:41
@bplommer bplommer merged commit 05213de into fd4s:series/1.x Jun 16, 2021
@keirlawson keirlawson deleted the producer-methods branch June 16, 2021 12:10
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.

3 participants