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

Pass Severity instead of String to Logger::Formatter #4369

Merged
merged 4 commits into from
May 7, 2017

Conversation

Sija
Copy link
Contributor

@Sija Sija commented May 3, 2017

Fixes #4355

Copy link

@mverzilli mverzilli left a comment

Choose a reason for hiding this comment

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

Thanks for this @Sija (that was fast! :)). On reviewing this I noticed there's zero documentation for the Formatter. Could we add that as part of this PR? This is a breaking change so having at least basic documentation will help users fix their programs faster.


private DEFAULT_FORMATTER = Formatter.new do |severity, datetime, progname, message, io|
io << severity[0] << ", [" << datetime << " #" << Process.pid << "] "
io << severity.rjust(5) << " -- " << progname << ": " << message
label = severity.unknown? ? "ANY" : severity.to_s

Choose a reason for hiding this comment

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

Why not just use "U" (for "Unknown")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copy-pasted this line from previous version. Why not just severity.to_s without branching into other symbols?

Choose a reason for hiding this comment

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

Hadn't noticed it was already like that. Then let's just leave it like this.

@Sija
Copy link
Contributor Author

Sija commented May 4, 2017

@mverzilli done.

@mverzilli
Copy link

@Sija I meant to document the formatter property, which is public. What do you think about this?

  1. Remove that comment from the alias definition
  2. Add this on top of the formatter property:
# Customizable `Proc` (with a reasonable default)
# which the Logger uses to format and print its entries.
#
# Use this setter to provide a custom formatter.
# The Logger will invoke it with the following arguments:
#  - severity: a `Logger::Severity`.
#  - datetime: `Time`, the entry's timestamp
#  - progname: `String`, the program name, if set when the logger was built
#  - message: `String`, the body of a message
#  - io: `IO`, the Logger's stream, to which you must write the output
# you want.
#
# Example:
#
# ```
# require "logger"
#
# logger = Logger.new(STDOUT)
# logger.progname = "YodaBot"
#
# logger.formatter = Logger::Formatter.new do |severity, datetime, progname, message, io|
#   label = severity.unknown? ? "ANY" : severity.to_s
#   io << label[0] << ", [" << datetime << " #" << Process.pid << "] "
#   io << label.rjust(5) << " -- " << progname << ": " << message
# end
#
# logger.warn("Fear leads to anger. Anger leads to hate. Hate leads to suffering.")
#
# # Prints to the console:
# # "W, [2017-05-06 18:00:41 -0300 #11927]  WARN --
# #  YodaBot: Fear leads to anger. Anger leads to hate. Hate leads to suffering."
# ```

Btw, I would have liked to directly add a commit to the PR without bothering you, but I'm not sure what the best workflow for doing it is :(.

@Sija Sija force-pushed the fix-logger-formatter-severity branch from e0d1635 to a286471 Compare May 6, 2017 22:24
@RX14
Copy link
Contributor

RX14 commented May 6, 2017

@mverzilli I think maintainers can push directly to most PRs now.

@Sija Sija force-pushed the fix-logger-formatter-severity branch from a286471 to e1186e0 Compare May 6, 2017 22:28
@Sija
Copy link
Contributor Author

Sija commented May 6, 2017

I've force-pushed the last commit with your docs added. In the future it's fine with me if you'd do it yourself. I've took a liberty to modify some interpunction and wording here & there if that's ok with you.

@Sija
Copy link
Contributor Author

Sija commented May 7, 2017

image
@RX14 with above option checked, they do.

@RX14
Copy link
Contributor

RX14 commented May 7, 2017

@Sija that box is checked by default now though, so I imagine most prs don't bother to change it.

@mverzilli
Copy link

Nice, it does work :).

@mverzilli mverzilli merged commit de6de1e into crystal-lang:master May 7, 2017
@mverzilli mverzilli added this to the Next milestone May 7, 2017
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.

3 participants