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

feat(bus): implement EventBus for RabbitMQ #6

Merged
merged 19 commits into from
Apr 10, 2022

Conversation

RomanReznichenko
Copy link
Contributor

closes #5

@codeclimate
Copy link

codeclimate bot commented Mar 22, 2022

Code Climate has analyzed commit 4cbb1c9 and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 4

The test coverage on the diff in this pull request is 98.4% (50% is the threshold).

This pull request will bring the total coverage in the repository to 99.0%.

View more on Code Climate.

@derevnjuk derevnjuk changed the title feat(core): provide EventBus based on RabitMQ feat(core): implement EventBus for RabbitMQ Mar 22, 2022
@derevnjuk derevnjuk added Size: epic The issue can lead to several PRs and should be broken down into Type: enhancement New feature or request. labels Mar 22, 2022
Copy link
Member

@derevnjuk derevnjuk left a comment

Choose a reason for hiding this comment

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

Please follow this guideline: #1 (comment)

According to BranchByAbstraction you should extract Bus abstraction into a separate PR. The same is true for RetryStategy, make sure that you utilize atomic PRs. while working on huge tasks. How to split huge tasks into small pieces you can find in LLD (see Proposal).

Moreover, Bus implementation should be to be inside a separate bus package according to LLD.

@derevnjuk derevnjuk removed the Size: epic The issue can lead to several PRs and should be broken down into label Mar 31, 2022
@derevnjuk derevnjuk changed the title feat(core): implement EventBus for RabbitMQ feat(bus): implement EventBus for RabbitMQ Mar 31, 2022
@derevnjuk derevnjuk force-pushed the master branch 6 times, most recently from 5a5a2c3 to 3f33f03 Compare April 1, 2022 08:20
Copy link
Member

@derevnjuk derevnjuk left a comment

Choose a reason for hiding this comment

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

There are a lot of critical issues, oddities, and bugs.

In addition to comments, please pay attention to the following ones:

packages/bus/src/Factories/RMQConnectionFacory.ts Outdated Show resolved Hide resolved
packages/bus/src/Factories/ConnectionFactory.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/Brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
@derevnjuk derevnjuk force-pushed the feat/bus branch 5 times, most recently from 7b261c3 to 1710338 Compare April 7, 2022 11:20
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/core/src/bus/Event.ts Outdated Show resolved Hide resolved
packages/core/src/bus/EventBus.ts Outdated Show resolved Hide resolved
packages/core/src/credentials-provider/Credentials.ts Outdated Show resolved Hide resolved
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
packages/bus/src/brokers/RMQEventBus.spec.ts Outdated Show resolved Hide resolved
@derevnjuk derevnjuk force-pushed the feat/bus branch 4 times, most recently from 1453e5b to f089d60 Compare April 7, 2022 19:40
Copy link
Member

@derevnjuk derevnjuk left a comment

Choose a reason for hiding this comment

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

@RomanReznichenko please ensure 100% test coverage of RMQEventBus. BTW it seems you forgot to implement HttpCommandDispatcher (you can start working on it in a separate PR)

@derevnjuk derevnjuk merged commit 5700409 into NeuraLegion:master Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement EventBus for RabbitMQ
2 participants