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

Add examples describing error handling in ActiveSupport::Notification… #34707

Merged
merged 1 commit into from
Dec 31, 2018

Conversation

xlts
Copy link
Contributor

@xlts xlts commented Dec 14, 2018

…s and ActiveSupport::LogSubscriber documentation files

Summary

This PR adds some missing code samples describing error handling in ActiveSupport::Notifications and ActiveSupport::LogSubscriber documentation files. It also includes some minor formatting updates to ActiveSupport::LogSubscriber docs.

# (<tt>ActiveSupport::Notifications::Event</tt>) to the sql method.
#
# Being an <tt>ActiveSupport::Notifications</tt> consumer,
# <tt>ActiveRecord::LogSubscriber</tt> exposes simple interface to check if
Copy link
Member

Choose a reason for hiding this comment

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

ActiveRecord::LogSubscriber exposes a simple interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx @gmcgibbon, this is now fixed -- it's always good to have some native speaker's feedback in PRs like this one. Anyway, this also made me realize that we should refer to ActiveSupport::LogSubscriber rather than ActiveRecord::LogSubscriber

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Please squash your commits and use [ci skip] to skip Travis

#
# Being an <tt>ActiveSupport::Notifications</tt> consumer,
# <tt>ActiveSupport::LogSubscriber</tt> exposes a simple interface to check if
# instrumented code raised an exception. It is common to log a different
Copy link
Member

Choose a reason for hiding this comment

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

I would also change this to "instrumented code raises an exception"

…s and ActiveSupport::LogSubscriber documentation files
@xlts
Copy link
Contributor Author

xlts commented Dec 18, 2018

@gmcgibbon I applied your suggestions. Let me know if something else needs updating before this one can be merged

@gmcgibbon
Copy link
Member

When testing the code sample, I don't see the subscriber actually output anything. This is because we're not calling any logging methods and just returning the strings, which is wrong. This is also present in the previous example, so I'd rather be consistently wrong for now. I'll fix this in a followup PR.

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

None yet

2 participants