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

Change Log emitters to not emit event when block output is nil #12000

Merged

Conversation

robacarp
Copy link
Contributor

fixes #11997

@straight-shoota
Copy link
Member

Could you please add a note about this to the API docs?

@straight-shoota
Copy link
Member

Happy to see your first PR here, btw. 😄

src/log/log.cr Outdated Show resolved Hide resolved
src/log/log.cr Outdated
# end
# }
# ```
def {{method}}(*, exception : Exception? = nil)
Copy link
Contributor Author

@robacarp robacarp Apr 15, 2022

Choose a reason for hiding this comment

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

@straight-shoota Good call. I added an example here as well. The documentation here felt like it could be improved a bit by restructuring this macro iteration in order to get the name of the severity instead of just the number:

Before:
image

After:
image

I didn't want to get too far into the weeds, I'm not sure if these out of scope changes are welcome in this pull request.

Copy link
Member

Choose a reason for hiding this comment

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

do / end would fit better. Multiline using { }, while valid, is more so used for single line blocks. E.g. Log.warn { "Nothing will be logged" if false } versus:

Log.warn do
  if false
    "Nothing will be logged"
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

It's a stylistic debate. { } is perfectly fine. It's often used to indicate that the block's output value matters.

# can be used. They expect a block that will evaluate to the message of the entry.
# To log a message use the `#trace`, `#debug`, `#info`, `#notice`, `#warn`,
# `#error`, and `#fatal` methods. They expect a block that will evaluate to the
# message of the entry:
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 switched this from:

"[methods] can be used" to "use the [methods]" which feels like it is more consistent with the technical writing tone I've observed elsewhere in the docs.

@robacarp robacarp force-pushed the prevent_log_emitting_with_nil branch 2 times, most recently from bb96092 to 60a0c2c Compare April 15, 2022 17:16
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

👍

src/log/log.cr Outdated Show resolved Hide resolved
@robacarp robacarp force-pushed the prevent_log_emitting_with_nil branch from 60a0c2c to dc97e15 Compare April 15, 2022 18:45
@robacarp robacarp force-pushed the prevent_log_emitting_with_nil branch from dc97e15 to bf5e952 Compare April 15, 2022 18:47
@straight-shoota straight-shoota added this to the 1.5.0 milestone Apr 20, 2022
@straight-shoota straight-shoota changed the title prevents log.[level] {nil} from emitting anything to logging backends Change Log emitters to not emit event when block output is nil Apr 27, 2022
@straight-shoota straight-shoota merged commit 4c7e5bf into crystal-lang:master Apr 27, 2022
@robacarp robacarp deleted the prevent_log_emitting_with_nil branch April 27, 2022 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way for Log.debug{} blocks to not print anything
6 participants