Add number_with_delimiter helper to format numbers to be more readable#25
Add number_with_delimiter helper to format numbers to be more readable#25okapusta wants to merge 3 commits intoappoptics:masterfrom
Conversation
okapusta
force-pushed
the
number-with-delimiter
branch
from
c8e941a to
cb10e43
Compare
okapusta
force-pushed
the
number-with-delimiter
branch
from
cb10e43 to
1141a82
Compare
akahn
left a comment
akahn
left a comment
There was a problem hiding this comment.
I love the idea of this but I don't understand why the implementation is specific to Slack. Also, I think passing the service from the service class to the output formatting module breaks the encapsulation of the service. A better object oriented design, in my view, would be to have a method on AppOptics::Services::Service that outputs the measurement, optionally overridden in a service-specific way.
| actual_value = measurement[:value] | ||
| formatted_value = AppOptics::Services::Numbers.format_for_threshold(threshold_value, actual_value) | ||
| "#{condition[:type]} threshold #{threshold(condition,measurement)} with value #{formatted_value}" | ||
| if service == AppOptics::Services::Service::Slack::SERVICE_NAME |
There was a problem hiding this comment.
Is there a reason that only Slack should benefit from this change?
There was a problem hiding this comment.
I can't find the conversation right now but it was pointed out that change in webhooks, for example, might break something for the customers so it was decided that we should only change Slack output.
2 participants
When an Alert notification is sent it looks for example like this:
If you look at the numbers it's really hard to figure out if 20000000 is 20,000,000 or 2,000,000 or 200,000,000. Added
number_with_delimiterhelper known from Rails to make numbers more readable.