-
Notifications
You must be signed in to change notification settings - Fork 245
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
Adds configurable option to set message format #204
Conversation
This allows the format of the messages produced by each formatter to be configured by specifying a format string in the .pronto.yml config file. Format can be overridden on a global basis by specifying the `format` key at the top-level of the file, or it can be overridden on a per-formatter basis by nesting `format` keys under the formatter's name (as defined in `Formatter::FORMATTERS` constant). If a formatter does not have a specific format defined in the config file, it will fall back to the global format, and then fall back to the default format (which is just `"%{msg}"`). Format uses a Ruby format string and makes all of the attributes of the `Message` object available for interpolation. The `TextFormatter` additionally adds a special `TextMessageDecorator` to add colorized and specially formatted values for file location and level. These were in the original formatter code so I wanted to keep them.
89c7fd7
to
bb31390
Compare
Looks like Travis ran into a 404 error trying to post Pronto results in the after_success hook 😳 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, just a couple of comments 👍
class TextFormatter | ||
include Colorizable | ||
class TextFormatter < Base | ||
class TextMessageDecorator < SimpleDelegator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm usually not a fan of classes within classes. Could you move outside to /formatter/text_message_decorator.rb
or give an argument why would you want to keep it inside the TextFormatter
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I nested it just because the decorator was only going to be used in this one place so I thought keeping it close improved readability. I don't feel strongly about it though so I'll reorganize it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmozuras moved TextMessageDecorator
to pronto/formatter/text_message_decorator.rb
colorize(level[0].upcase, color) | ||
def format(messages, _, _) | ||
messages.map do |message| | ||
(config.message_format(self.class.name) % TextMessageDecorator.new(message).to_h).strip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you split this line into multiple for readability? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split up 💅
|
||
def to_h | ||
original = __getobj__.to_h | ||
original[:color_level] = format_level(__getobj__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand pros and cons: why did you decide to leave color_level
and color_location
off README? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to README 😃
@aergonaut will look into it 👌 |
@mmozuras I've updated the code according to your comments, please have a look! |
@aergonaut perfect, thank you! 🙇 |
@aergonaut awesome addition! 👏 👍 |
Adds configurable option to set message format
This allows the format of the messages produced by each formatter to be configured by specifying a format string in the .pronto.yml config file.
Format can be overridden on a global basis by specifying the
format
key at the top-level of the file, or it can be overridden on a per-formatter basis by nestingformat
keys under the formatter's name (as defined inFormatter::FORMATTERS
constant). If a formatter does not have a specific format defined in the config file, it will fall back to the global format, and then fall back to the default format (which is just"%{msg}"
).Format uses a Ruby format string and makes all of the attributes of the
Message
object available for interpolation. TheTextFormatter
additionally adds a specialTextMessageDecorator
to add colorized and specially formatted values for file location and level. These were in the original formatter code so I wanted to keep them.The Null, JSON, and Checkstyle formatters are excluded from this PR because those aren't really outputting a human-readable format, but rather transforming the message into some other format for some other system to consume. So specifying a format string for these didn't make sense to me.