refactor log router API to use named args and clearer names for those args
Tyler Riddle [Tue, 8 Jan 2013 22:32:35 +0000 (14:32 -0800)]
Makefile.PL
lib/Log/Contextual.pm
lib/Log/Contextual/Role/Router.pm
lib/Log/Contextual/Router.pm
t/lib/TestExporter.pm [new file with mode: 0644]
t/lib/TestRouter.pm [new file with mode: 0644]
t/router_api.t [new file with mode: 0644]

index 57bd0d9..fd0df7d 100644 (file)
@@ -6,7 +6,7 @@ use warnings FATAL => 'all';
 perl_version '5.006';
 all_from 'lib/Log/Contextual.pm';
 requires 'Data::Dumper::Concise';
-requires 'Exporter::Declare' => 0.105;
+requires 'Exporter::Declare' => 0.106;
 requires 'Carp';
 requires 'Scalar::Util';
 requires 'Moo';
index 1af4517..7d26b22 100644 (file)
@@ -51,11 +51,12 @@ sub before_import {
    my ($class, $importer, $spec) = @_;
    my $router = $class->router;
    my $exports = $spec->exports;
+   my %router_args = (exporter => $class, target => $importer, arguments => $spec->argument_info);
 
    die 'Log::Contextual does not have a default import list'
       if $spec->config->{default};
 
-   $router->before_import(@_);
+   $router->before_import(%router_args);
 
    $spec->add_export('&set_logger', sub {
       my $router = $class->router;
@@ -80,22 +81,18 @@ sub before_import {
       if ($spec->config->{log}) {
          $spec->add_export("&log_$level", sub (&@) {
             my ($code, @args) = @_;
-            $router->handle_log_request({
-               controller => $class,
-               package => scalar(caller),
-               caller_level => 1,
-               level => $level,
-            }, $code, @args);
+            $router->handle_log_request(
+               exporter => $class, caller_package => scalar(caller), caller_level => 1,
+               message_level => $level, message_sub => $code, message_args => \@args,
+            );
             return @args;
          });
          $spec->add_export("&logS_$level", sub (&@) {
             my ($code, @args) = @_;
-            $router->handle_log_request({
-               controller => $class,
-               package => scalar(caller),
-               caller_level => 1,
-               level => $level,
-            }, $code, @args);
+            $router->handle_log_request(
+               exporter => $class, caller_package => scalar(caller), caller_level => 1,
+               message_level => $level, message_sub => $code, message_args => \@args,
+            );
             return $args[0];
          });
       }
@@ -106,12 +103,10 @@ sub before_import {
                local $_ = (@_?Data::Dumper::Concise::Dumper @_:'()');
                &$code;
             };
-            $router->handle_log_request({
-               controller => $class,
-               package => scalar(caller),
-               caller_level => 1,
-               level => $level,
-            }, $wrapped, @args);
+            $router->handle_log_request(
+               exporter => $class, caller_package => scalar(caller), caller_level => 1,
+               message_level => $level, message_sub => $wrapped, message_args => \@args,
+            );
             return @args;
          });
          $spec->add_export("&DlogS_$level", sub (&$) {
@@ -120,19 +115,21 @@ sub before_import {
                local $_ = Data::Dumper::Concise::Dumper($_[0]);
                &$code;
             };
-            $router->handle_log_request({
-               controller => $class,
-               package => scalar(caller),
-               caller_level => 1,
-               level => $level,
-            }, $wrapped, $ref);
+            $router->handle_log_request(
+               exporter => $class, caller_package => scalar(caller), caller_level => 1,
+               message_level => $level, message_sub => $wrapped, message_args => [ $ref ],
+            );
             return $ref;
          });
       }
    }
 }
 
-sub after_import { $_[0]->router->after_import(@_) }
+sub after_import {
+   my ($class, $importer, $spec) = @_;
+   my %router_args = (exporter => $class, target => $importer, arguments => $spec->argument_info);
+   $class->router->after_import(%router_args);
+}
 
 1;
 
@@ -591,22 +588,23 @@ them.  For a basic example see L<Log::Contextual::SimpleLogger>.
 
 =head1 LOG ROUTING
 
-Inbetween the loggers and the log methods is a log router that is responsible for
+In between the loggers and the log functions is a log router that is responsible for
 finding a logger to handle the log event and passing the log information to the
-logger. This relationship is described in the documentation for
-C<Log::Contextual::Role::Router>.
+logger. This relationship is described in the documentation for C<Log::Contextual::Role::Router>.
 
-C<Log::Contextual> and subclasses by default share a router singleton that implements
-the with_logger() and set_logger() methods and also respects the -logger, -package_logger,
-and -default_logger import options with their associated default value methods. The router
-singleton is available as the return value of the router() method. Users of Log::Contextual
-may overload the router() method to return instances of custom log routers that could for
-example work with loggers that use a different interface.
+C<Log::Contextual> and packages that extend it will by default share a router singleton that
+implements the with_logger() and set_logger() functions and also respects the -logger,
+-package_logger, and -default_logger import options with their associated default value
+functions. The router singleton is available as the return value of the router() function. Users
+of Log::Contextual may overload router() to return instances of custom log routers that
+could for example work with loggers that use a different interface.
 
-=head1 AUTHORS
+=head1 AUTHOR
 
 frew - Arthur Axel "fREW" Schmidt <frioux@gmail.com>
 
+=head1 CONTRIBUTORS
+
 triddle - Tyler Riddle <t.riddle@shadowcat.co.uk>
 
 =head1 DESIGNER
index 0d8643a..b9623fc 100644 (file)
@@ -16,7 +16,7 @@ Log::Contextual::Role::Router - Abstract interface between loggers and logging c
 
 =head1 SYNOPSIS
 
-  package Custom::Logging::Router;
+  package MyApp::Log::Router;
   
   use Moo;
   use Log::Contextual::SimpleLogger;
@@ -30,41 +30,49 @@ Log::Contextual::Role::Router - Abstract interface between loggers and logging c
   }
   
   sub before_import {
-     my ($self, $log_class, $importer, $spec) = @_;
-     print STDERR "Package '$importer' will import '$log_class'\n";
+     my ($self, %export_info) = @_;
+     my $exporter = $export_info{exporter};
+     my $target = $export_info{target};
+     print STDERR "Package '$target' will import from '$exporter'\n";
   }
 
   sub after_import {
-     my ($self, $log_class, $importer, $spec) = @_;
-     print STDERR "Package '$importer' has imported '$log_class'\n";
+     my ($self, %export_info) = @_;
+     my $exporter = $export_info{exporter};
+     my $target = $export_info{target};
+     print STDERR "Package '$target' has imported from '$exporter'\n";
   }
 
   sub handle_log_request {
-    my ($self, $metadata, $log_code_block, @args) = @_;
-    my $log_level_name = $metadata->{level};
-    my $logger = $self->logger;
-    my $is_active = $logger->can("is_$log_level_name");
-    
-    return unless defined $is_active && $logger->$is_active;
-    my $log_message = $log_code_block->(@args);
-    $logger->$log_level_name($log_message);
+     my ($self, %message_info) = @_;
+     my $log_code_block = $message_info{message_sub};
+     my $args = $message_info{message_args};
+     my $log_level_name = $message_info{message_level};
+     my $logger = $self->logger;
+     my $is_active = $logger->can("is_${log_level_name}");
+     
+     return unless defined $is_active && $logger->$is_active;
+     my $log_message = $log_code_block->(@$args);
+     $logger->$log_level_name($log_message);
   }
 
-  package Custom::Logging::Class;
+  package MyApp::Log::Contextual;
 
   use Moo;
-
+  use MyApp::Log::Router;
+  
   extends 'Log::Contextual';
 
-  #Almost certainly the router object should be a singleton
+  #This example router is a singleton
   sub router {
-     our $Router ||= Custom::Logging::Router->new
+     our $Router ||= MyApp::Log::Router->new
   }
 
   package main;
 
-  use strictures;
-  use Custom::Logging::Class qw(:log);
+  use strict;
+  use warnings;
+  use MyApp::Log::Contextual qw(:log);
   
   log_info { "Hello there" };
 
@@ -76,15 +84,15 @@ Log::Contextual has three parts
 
 =item Export manager and logging method generator
 
-These tasks are handled by the C<Log::Contextual> class.
+These tasks are handled by the C<Log::Contextual> package.
 
 =item Logger selection and invocation
 
-The log methods generated and exported by Log::Contextual call a method
-on a log router object which is responsible for invoking any loggers that should
-get an opportunity to receive the log message. The C<Log::Contextual::Router>
-class implements the set_logger() and with_logger() methods as well as uses the
-arg_ prefixed methods to configure itself and provide the standard C<Log::Contextual>
+The logging functions generated and exported by Log::Contextual call a method
+on an instance of a log router object which is responsible for invoking any loggers
+that should get an opportunity to receive the log message. The C<Log::Contextual::Router>
+class implements the set_logger() and with_logger() functions as well as uses the
+arg_ prefixed functions to configure itself and provide the standard C<Log::Contextual>
 logger selection API.
 
 =item Log message formatting and output
@@ -100,58 +108,53 @@ block and passing the generated message to the logging object's log method.
 
 =over 4
 
-=item before_import($self, $log_class, $importer, $spec)
+=item before_import($self, %import_info)
 
-=item after_import($self, $log_class, $importer, $spec)
+=item after_import($self,  %import_info)
 
 These two required methods are called with identical arguments at two different places
 during the import process. The before_import() method is invoked prior to the logging
-methods being exported into the consuming packages namespace. The after_import() method
-is called when the export is completed but before control returns to the package that
-imported the class.
+subroutines being exported into the target package and after_import() is called when the
+export is completed but before control returns to the package that imported the API.
 
-The arguments are as follows:
+The arguments are passed as a hash with the following keys:
 
 =over 4
 
-=item $log_class
+=item exporter
 
-This is the package name of the subclass of Log::Contextual that has been imported. It can
-also be 'Log::Contextual' itself. In the case of the synopsis the value in $log_class would be
-'Custom::Logging::Class'.
+This is the name of the package that has been imported. It can also be 'Log::Contextual' itself. In
+the case of the synopsis the value for exporter would be 'MyApp::Log::Contextual'.
 
-=item $importer
+=item target
 
-This is the package name that is importing the logging class. In the case of the synopsis the
+This is the package name that is importing the logging API. In the case of the synopsis the
 value would be 'main'.
 
-=item $spec
+=item arguments
 
-This is the import specification that is being used when exporting methods to $importer. The
-value is an unmodified C<Exporter::Declare::Specs> object.
-
-=back
+This is a hash reference containing the configuration values that were provided for the import. 
+The key is the name of the configuration item that was specified without the leading hyphen ('-').
+For instance if the logging API is imported as follows
 
-=item handle_log_request($self, $info, $generator, @args)
+  use Log::Contextual qw( :log ), -logger => Custom::Logger->new({ levels => [qw( debug )] });
 
-This method is called by C<Log::Contextual> when a log event happens. The arguments are as
-follows:
+then $import_info{arguments}->{logger} would contain that instance of Custom::Logger.
 
-=over 4
+=back
 
-=item $info
+=item handle_log_request($self, %message_info)
 
-This is the metadata describing the log event. The value is a hash reference with the following
-keys:
+This method is called by C<Log::Contextual> when a log event happens. The arguments are passed
+as a hash with the following keys
 
 =over 4
 
-=item controller
+=item exporter
 
-This is the name of the Log::Contextual subclass (or 'Log::Contextual' itself) that created
-the logging methods used to generate the log event.
+This is the name of the package that created the logging methods used to generate the log event.
 
-=item package
+=item caller_package
 
 This is the name of the package that the log event has happened inside of.
 
@@ -160,22 +163,20 @@ This is the name of the package that the log event has happened inside of.
 This is an integer that contains the value to pass to caller() that will provide
 information about the location the log event was created at.
 
-=item level
+=item log_level
 
 This is the name of the log level associated with the log event.
 
-=back
-
-=item $generator
+=item message_sub
 
-This is the message generating block associated with the log event passed as a subref. If
-the logger accepts the log request the router should execute the generator to create
+This is the message generating code block associated with the log event passed as a subref. If
+the logger accepts the log request the router should execute the subref to create
 the log message and then pass the message as a string to the logger.
 
-=item @args
+=item message_args
 
-This is the arguments provided to the log block passed through completely unmodified. When
-invoking the generator method it will almost certainly be expecting these argument values
+This is an array reference that contains the arguments given to the message generating code block.
+When invoking the message generator it will almost certainly be expecting these argument values 
 as well.
 
 =back
index 08e4577..4aa05f7 100644 (file)
@@ -16,19 +16,21 @@ eval {
 sub before_import { }
 
 sub after_import {
-   my ($self, $controller, $importer, $spec) = @_;
-   my $config = $spec->config;
-
-   if (my $l = $controller->arg_logger($config->{logger})) {
-      $self->set_logger($l)
+   my ($self, %import_info) = @_;
+   my $exporter = $import_info{exporter};
+   my $target = $import_info{target};
+   my $config = $import_info{arguments};
+   
+   if (my $l = $exporter->arg_logger($config->{logger})) {
+      $self->set_logger($l);
    }
 
-   if (my $l = $controller->arg_package_logger($config->{package_logger})) {
-      $self->_set_package_logger_for($importer, $l)
+   if (my $l = $exporter->arg_package_logger($config->{package_logger})) {
+      $self->_set_package_logger_for($target, $l);
    }
 
-   if (my $l = $controller->arg_default_logger($config->{default_logger})) {
-      $self->_set_default_logger_for($importer, $l)
+   if (my $l = $exporter->arg_default_logger($config->{default_logger})) {
+      $self->_set_default_logger_for($target, $l);
    }
 }
 
@@ -78,10 +80,9 @@ sub _set_package_logger_for {
 }
 
 sub get_loggers {
-   my ($self, $info) = @_;
-
-   my $package = $info->{package};
-
+   my ($self, %info) = @_;
+   my $package = $info{caller_package};
+   my $log_level = $info{message_level};
    my $logger = (
       $_[0]->{Package_Logger}->{$package} ||
       $_[0]->{Get_Logger} ||
@@ -89,25 +90,23 @@ sub get_loggers {
       die q( no logger set!  you can't try to log something without a logger! )
    );
 
-   my %info = %$info;
-
    $info{caller_level}++;
-
    $logger = $logger->($package, \%info);
 
-   return $logger if $logger->${\"is_${\$info->{level}}"};
+   return $logger if $logger->${\"is_${log_level}"};
    return ();
 }
 
 sub handle_log_request {
-   my ($self, $info, $generator, @args) = @_;
-
-   my %info = %$info;
+   my ($self, %message_info) = @_;
+   my $generator = $message_info{message_sub};
+   my $args = $message_info{message_args};
+   my $log_level = $message_info{message_level};
 
-   $info{caller_level}++;
+   $message_info{caller_level}++;
 
-   foreach my $logger ($self->get_loggers(\%info)) {
-      $logger->${\$info->{level}}($generator->(@args));
+   foreach my $logger ($self->get_loggers(%message_info)) {
+      $logger->$log_level($generator->(@$args));
    }
 }
 
diff --git a/t/lib/TestExporter.pm b/t/lib/TestExporter.pm
new file mode 100644 (file)
index 0000000..ba7ab95
--- /dev/null
@@ -0,0 +1,12 @@
+package TestExporter;
+
+use Moo;
+use TestRouter;
+
+extends 'Log::Contextual';
+
+sub router {
+   our $Router ||= TestRouter->new
+}
+
+1;
\ No newline at end of file
diff --git a/t/lib/TestRouter.pm b/t/lib/TestRouter.pm
new file mode 100644 (file)
index 0000000..67e57c1
--- /dev/null
@@ -0,0 +1,25 @@
+package TestRouter;
+
+use Moo;
+use Log::Contextual::SimpleLogger;
+
+with 'Log::Contextual::Role::Router';
+
+has captured => (is => 'ro', default => sub { {} });
+
+sub before_import {
+   my ($self, %export_info) = @_;
+   $self->captured->{before_import} = \%export_info;
+}
+
+sub after_import {
+   my ($self, %export_info) = @_;
+   $self->captured->{after_import} = \%export_info;
+}
+
+sub handle_log_request {
+   my ($self, %message_info) = @_;
+   $self->captured->{message} = \%message_info;
+}
+
+1;
\ No newline at end of file
diff --git a/t/router_api.t b/t/router_api.t
new file mode 100644 (file)
index 0000000..276b918
--- /dev/null
@@ -0,0 +1,33 @@
+use strict;
+use warnings;
+use Test::More;
+use lib 't/lib';
+
+use TestExporter qw(:log), -logger => 'logger value', -default_logger => 'default logger value',
+   -package_logger => 'package logger value';
+
+my @test_args = qw( some argument values );
+log_info { "Ignored value" } @test_args;
+
+my $results = TestExporter->router->captured;
+my %export_info = (
+   exporter => 'TestExporter', target => 'main', arguments => {
+      logger => 'logger value', default_logger => 'default logger value',
+      package_logger => 'package logger value'
+   },
+);
+my %message_info = (
+   exporter => 'TestExporter', caller_package => 'main', caller_level => 1,
+   message_level => 'info', message_args => \@test_args,
+);
+
+is_deeply($results->{before_import}, \%export_info, 'before_import() values are correct');
+is_deeply($results->{after_import}, \%export_info, 'after_import() values are correct');
+
+#can't really compare the sub ref value so make sure it exists and is the right type
+#and remove it for the later result check
+my $message_block = delete $results->{message}->{message_sub};
+is(ref $message_block, 'CODE', 'handle_log_request() got a sub ref for the message generator');
+is_deeply($results->{message}, \%message_info, 'handle_log_request() other values are correct');
+
+done_testing;
\ No newline at end of file