Remove the ability to configure multiple notifiers #429
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyrylo
force-pushed
the
one-notifier-one-config
branch
from
97f16a2 to
cdd2fb4
Compare
This feature starts getting in the way of future development of the library: * We have to maintain a complex API to support different configurations for many notifiers (we have 3 now vs 1 when we were releasing this functionality). We want a simple API * It's hard to pass the config object around. If a component that sits at the bottom layer of the library architecture suddenly needs to read some config value, we have to pass it through all the layers, from the top to the bottom. This was a compromise since the very beginning but back then it was easy to maintain it (although it always bugged me). We now rapidly add new features and this approach does not scale well - too much clutter * Classes know more than they really need. As a result, testing becomes harder since we need to inject the config (sometimes with "correct" values). An alternative way would be exposing the config through `Airbrake.notifiers[:default][:notice].config.config_value`. This only adds to the clutter since the API becomes more complex and less malleable. On the other hand, this feature is likely not used by 90% of our customers. We have a solid Rails workflow that never involves more than 1 notifier. If people need another notifier, they can easily instantiate it directly through `Airbrake::NoticeNotifier.new`.
kyrylo
force-pushed
the
one-notifier-one-config
branch
from
cdd2fb4 to
3cf1e0c
Compare
This was referenced May 24, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This feature starts getting in the way of future development of the library:
We have to maintain a complex API to support different configurations for many
notifiers (we have 3 now vs 1 when we were releasing this functionality). We
want a simple API
It's hard to pass the config object around. If a component that sits at the
bottom layer of the library architecture suddenly needs to read some config
value, we have to pass it through all the layers, from the top to the
bottom. This was a compromise since the very beginning but back then it was
easy to maintain it (although it always bugged me). We now rapidly add new
features and this approach does not scale well - too much clutter
Classes know more than they really need. As a result, testing becomes harder
since we need to inject the config (sometimes with "correct" values).
An alternative way would be exposing the config through
Airbrake.notifiers[:default][:notice].config.config_value. This only adds tothe clutter since the API becomes more complex and less malleable.
On the other hand, this feature is likely not used by 90% of our customers. We
have a solid Rails workflow that never involves more than 1 notifier.