replace @target.logger with logger method. #4728

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@hindenbug
Contributor

No description provided.

@vijaydev
Member

Why are you resubmitting the same PR once every few days?? #4600 #4622 #4638

@hindenbug
Contributor

@vijaydev I am sorry but there was some problem with my fork so had to delete them, reopen the PR.

@vijaydev
Member

Why is this change needed?

@hindenbug
Contributor

@vijaydev as there is already a logger method defined which returns @target.logger , so instead of calling @target.logger again, it makes sense to use the logger method.

@sachin87
Contributor

@railsaholic, u r right!!

@hindenbug
Contributor

@josevalim Is it good enough? Thanks

@Vasfed
Vasfed commented Mar 16, 2012

Actually this only adds one more method call without any profit.
Plus if logger method is redefined by user, but logger? is not for some reason - this can lead to bugs as checked and returned objects can be different

@carlosantoniodasilva

@railsaholic thank you for your patch, but I believe the method is clearer by using @target.logger since it already tests for @target.respond_to?(:logger) in the same line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment