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

Decoupled notification service #844

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

juherr
Copy link
Contributor

@juherr juherr commented Jun 18, 2022

The objective of having a decoupled notification service is to allow adding some other notification service (ie: a SlackNotificationService) without any change in the business code.

@goekay
Copy link
Member

goekay commented Jun 20, 2022

thanks @juherr!

@goekay goekay merged commit 1889fb1 into steve-community:master Jun 20, 2022
@juherr juherr deleted the feature/decoupled-notifications branch June 20, 2022 20:34
@juherr
Copy link
Contributor Author

juherr commented Jun 20, 2022

@goekay thank for the merge. I didn't test the change very well and didn't check if unit tests are already covering the feature.
Please, tell me if I can improve it.

I will propose some other changes about notifications soon What is your favorite process? Is draft a good way for you to discuss about changes?

@goekay
Copy link
Member

goekay commented Jun 21, 2022

I didn't test the change very well and didn't check if unit tests are already covering the feature. Please, tell me if I can improve it.

i think the changes of this PR were straightforward and simple enough not to require tests. but usually, yeah, the more tests the better. you can keep that for future in mind. i am also trying to improve the situation when i add/change something.

I will propose some other changes about notifications soon What is your favorite process? Is draft a good way for you to discuss about changes?

i don't have a process. you can make a PR and we can discuss it as part of the review. or, if you want to discuss the idea without the actual effort of executing the idea, we can use an issue.

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.

2 participants