Allow logger to be lazy and converted on first use - #14
Conversation
mberlanda
left a comment
mberlanda
left a comment
There was a problem hiding this comment.
Overall I trust you 😄
Would it be possible to add a test and/or an example of configuration with and without proc? This would make the change more documented
|
If you have a look at the labels there is a Usually, I change it when I've added tests and in some way or form verified that the code does what I expect it to do. |
There was a problem hiding this comment.
Pull request overview
This PR updates Brpoplpush::RedisScript::Config to support lazy logger assignment (via a Proc) and to evaluate/convert that proc into a real Logger on first use, addressing load-order issues when configuring unique jobs.
Changes:
- Replace the
attr_reader :loggerwith aloggermethod that evaluates a storedProcon first call. - Allow
logger=to accept either aLoggerinstance or aProcproducing one. - Update validation/error messaging around logger assignment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def logger | ||
| # Convert to a regular logger on first call | ||
| @logger = @logger.call if @logger.is_a?(Proc) | ||
| @logger | ||
| end |
| # @raise [ArgumentError] when given argument isn't a Logger | ||
| # | ||
| # @return [Logger] |
| # | ||
| def logger=(obj) | ||
| raise ArgumentError, "#{obj} should be a Logger" unless obj.is_a?(Logger) | ||
| raise ArgumentError, "#{obj} should be a Logger or a proc" unless obj.is_a?(Logger) || obj.is_a?(Proc) |
| def logger | ||
| # Convert to a regular logger on first call | ||
| @logger = @logger.call if @logger.is_a?(Proc) |
The problem otherwise is that there is a load order problem in unique jobs. At least this way I can assign the logger regardless of how files are loaded.