-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update logging documentation #2035
Conversation
@msathieu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4 and @maurogeorge to be potential reviewers. |
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.
Please change to recommend putting in an initializer. I'd also advocate requiring the lib in that spot, rather than relying on something upstream doing it.
docs/general/logging.md
Outdated
You can also disable the logger, just put this in `config/application.rb`: | ||
|
||
```ruby | ||
ActiveSupport::Notifications.unsubscribe(ActiveModelSerializers::Logging::RENDER_EVENT) |
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.
It's a better practice to put this in an initializer in a Rails app.
config/initializers/active_model_serializers.rb
is the usual location.
require 'active_model_serializers'
ActiveSupport::Notifications.unsubscribe(ActiveModelSerializers::Logging::RENDER_EVENT)
and perhaps reference the relevant railtie https://github.com/rails-api/active_model_serializers/blob/0-10-stable/lib/active_model_serializers/railtie.rb#L23-L27 ?
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.
Why requiring it, that's done by Rails.
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.
@msathieu not exactly. It's actually the call to Bundle.require
at the top of most config/application.rb
and then only if the gem doesn't have require: false
.
Also, relying on autoloading of your project's classes is pretty much required for code reloading to work. But that doesn't really apply to gems. It's a good idea to require a gem before you use it. But this opinion is controversial enough that I won't tell you to include it :)
@msathieu Thanks so much for this and the quick turnaround! |
@bf4 Updated it to use an initializer |
@bf4 I have updated it to require active_model_serializers in the initializer |
@msathieu would you mind adding a changelog entry for yourself under 'misc'? |
@bf4 I've made an entry. |
Include documentation on how to disable the logging.
#2029 @bf4