Config logger #1611
Config logger #1611
Conversation
| @@ -32,7 +32,7 @@ class UiController < ActionController::Base; end | |||
| boot_dummy | |||
|
|
|||
| Rails.application.routes.draw do | |||
| mount(Quilt::Engine, at: '/quilt') | |||
| mount(::Quilt::Engine, at: '/quilt') | |||
There was a problem hiding this comment.
what does the prefix :: mean?
There was a problem hiding this comment.
QuiltRails::Quilt gets defined by a controller test, so :: makes sure the toplevel Quilt module gets referenced.
| @@ -11,6 +11,7 @@ def initialize | |||
|
|
|||
| self.react_server_host = "#{react_server_ip}:#{react_server_port}" | |||
| self.react_server_protocol = ENV['REACT_SERVER_PROTOCOL'] || "http" | |||
| self.logger = ::Logger.new($stdout) | |||
There was a problem hiding this comment.
looks weird to me to set this logger inside the configuration. Quilt::Configuration.logger.log :/
Why not do like Rails and just set a class variable in the Quilt module directly ?
There was a problem hiding this comment.
We already have a configuration object with a configuration pattern though. The new API is Quilt.logger which delegates to Quilt.configuration.logger. As long as the API is the same I don't see any problems.
There was a problem hiding this comment.
No problem indeed but since you are delegating Quilt.logger to the configuration I don't see the point of not adding that directly in the Quilt module. That being said you also can't use delegate because that gem don't depend on active support
There was a problem hiding this comment.
No problem indeed but since you are delegating Quilt.logger to the configuration I don't see the point of not adding that directly in the Quilt module.
I mainly want to maintain the contract of the configuration object. We would have to deprecate Quilt.configuration too if we want configuration accessors to live on the root module.
That being said you also can't use delegate because that gem don't depend on active support
It depends on railties which depends on active support, but I could add a dependency in the gemspec to make that more apparent.
gmcgibbon
force-pushed
the
config_logger
branch
from
ab36c77 to
868db20
Compare
gmcgibbon
force-pushed
the
config_logger
branch
from
868db20 to
e084528
Compare
Description
Deprecates
Quilt::Logger.log, adds more standardQuilt.logger.This allows us to switch out test logger easily so running tests doesn't log anything. A lot of work for a small change, but this is a more standard method of logging for gems.
Type of change
quilt_railsPatch: Bug (non-breaking change which fixes an issue)quilt_railsMinor: New feature (non-breaking change which adds functionality)quilt_railsMajor: Breaking change (fix or feature that would cause existing functionality to not work as expected)Checklist