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

[PLATFORM-1085]: Add some event bus specific impl #155

Merged

Conversation

cottinisimone
Copy link
Contributor

@cottinisimone cottinisimone requested a review from a team as a code owner May 11, 2023 13:08
@cottinisimone cottinisimone marked this pull request as draft May 11, 2023 13:08
use crate::Aggregate;

#[derive(TypedBuilder)]
pub struct RabbitEventBusConfig<'a> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly not so sure about if this builder patter with typed-builder makes sense or is better to implement a manual builder..

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this builder has 9000 fields I don't see why not use typed-builder?

Copy link
Contributor

@oliverbrowneprima oliverbrowneprima left a comment

Choose a reason for hiding this comment

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

This looks good to me - an example or two and some tests (maybe examples used as tests?) might be nice, but since this is all work in progress, I don't think it's a blocker

use crate::Aggregate;

#[derive(TypedBuilder)]
pub struct RabbitEventBusConfig<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this builder has 9000 fields I don't see why not use typed-builder?

cpiemontese
cpiemontese previously approved these changes May 15, 2023
@cottinisimone cottinisimone requested a review from cpiemontese May 16, 2023 12:30
topic: String,
request_timeout: Duration,
error_handler: Box<dyn Fn(KafkaEventBusError) + Send + Sync>,
_phantom: PhantomData<A>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the phantom data really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because KafkaEventBus takes that A generic that is not used in any field of the struct otherwise..

tests/rabbit/mod.rs Outdated Show resolved Hide resolved
@cottinisimone cottinisimone requested a review from cpiemontese May 17, 2023 08:10
cpiemontese
cpiemontese previously approved these changes May 17, 2023
@cottinisimone
Copy link
Contributor Author

@ollaw can you please sign the drone.yml file?

@ollaw
Copy link

ollaw commented May 17, 2023

Don't you have a DevOps champion to do that ?
(don't want to look rude, just understanding)

@cottinisimone cottinisimone marked this pull request as ready for review May 22, 2023 08:01
Copy link
Member

@visd0m visd0m left a comment

Choose a reason for hiding this comment

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

Not a blocking comment, I certainly lost track of some changes scattered along the series of restructuring prs.

sequence_number: 1,
};

bus.publish(&store_event).await;
Copy link
Member

Choose a reason for hiding this comment

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

It is not clear to me if bus.publish(&store_event).await; is automatically called by some part of the library or if we expect the library user to call it (I guess I got lost in in the changes of this series of prs 😓). Would it make sense to add an example for the event bus flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've already added that in another PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok 👍. So I will wait to see all together.

Copy link
Member

@visd0m visd0m left a comment

Choose a reason for hiding this comment

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

Looking forward to see all the pieces together 👀.

@cottinisimone cottinisimone merged commit 4a07474 into master May 29, 2023
@cottinisimone cottinisimone deleted the PLATFORM-1085/task/add-some-event-bus-specific-impl branch May 29, 2023 07:31
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

Successfully merging this pull request may close these issues.

5 participants