Beefy Boxes and Bandwidth Generously Provided by pair Networks
Do you know where your variables are?
 
PerlMonks  

Re^2: Advice on style

by afoken (Prior)
on Nov 26, 2010 at 10:13 UTC ( #873817=note: print w/ replies, xml ) Need Help??


in reply to Re: Advice on style
in thread Advice on style

I didn't know that require Exporter; our @ISA = qw(Exporter); is outdated code.

And how should you know that?

The synopsis of Exporter still (as of v5.63 found on CPAN) has this code as the very first example. The shorter and cleaner use Exporter 'import'; is the second example, and no word in the synopsis explains the differences. Even in the "Good Practices" section linked from the synopsis, all of the examples inherit from Exporter. Importing Exporter::import is explained in just four sentences hidden in "Advanced Features" -> "Exporting without inheriting from Exporter".

I think the Exporter documentation needs some patches, explaining the difference. It also should tell people not to inherit from Exporter in new code. Perhaps all of the inheritance stuff should be moved to a "legacy usage" section.

Alexander

--
Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)


Comment on Re^2: Advice on style
Select or Download Code
Replies are listed 'Best First'.
Re^3: Advice on style
by yitzchak (Sexton) on Nov 26, 2010 at 20:40 UTC

    On the other hand, there's something to be said for reading all the documentation of a module you want to use.

    You find little gems like: use Some::Exporting::Module '/./';

Re^3: Advice on style
by Discipulus (Priest) on Jul 07, 2015 at 08:05 UTC
    i'm a little late for this threads.. I never understood such thing about Exporter.. anyway the docs have been updated in this while:
    Exporting-Without-Inheriting-from-Exporter

    thanks
    L*


    There are no rules, there are no thumbs..
    Reinvent the wheel, then learn The Wheel; may be one day you reinvent one of THE WHEELS.

      Yes, the documentation has been updated, but only a little bit. See http://search.cpan.org/diff?from=Exporter-5.63&to=Exporter-5.72&w=1#lib/Exporter.pm, comparing 2008 with 2015. Still most part of the documentation uses inheritance, its presented as the way to do it, whereas "Exporting without inhertiting" looks like an exotic way that more or less accidentally also works.

      The problem is that this is quite true, from the viewpoint of the Exporter authors. use Exporter qw( import ); is a nice shortcut, but to use all features of Exporter (Module->export_to_level(...), module version checking as callback to a VERSION() method, export_fail() callback, and for most of the inner workings of Exporter, you currently need to inherit from Exporter.

      From the viewpoint of a common user of Exporter, the situation is quite different: "All I want is to export foo by default, and bar and baz when requested." Yes, there may be users that need export_fail() or export_to_level(), but I think they are rare.

      (On my Linux box: 30 out of 3425 modules use export_to_level(), 6 out of 3425 use export_fail(); that includes Exporter.pm and Exporter/Heavy.pm)

      The funny thing with Exporter is that OOP modules should not use it ("As a general rule, if the module is trying to be object oriented then export nothing."), but forces (up to perl v5.8.2) non-OOP modules that want to export names to use OOP for exporting. For me, this looks like an artefact of how Exporter works internally. It abuses OOP to implicitly pass the name of the exporting module around. The "exporting without inheriting" change to import in perl v5.8.3 clearly shows that this is not necessary, but the documentation is still written as if you could hardly avoid inheriting.

      I have never seen a module extending Exporter to enhance exporting for other modules, and the current code of Exporter should make it quite hard to extend it, as it freely mixes method calls, function calls, and goto.

      I think that all callbacks into the exporting modules should be or should have been function calls, not method calls. Those callbacks are highly specific for the exporting module, and inheriting them from another module is quite likely a bug. And btw, modules inheriting from other modules are OOP modules and should not use Exporter at all, according to its own documentation.

      I'm aware that changing the behaviour of Exporter may cause tons of errors in existing code, so Exporter must be changed very carefully. Changing the documentation does not break any code. Documenting use Exporter qw( import ); our @EXPORT=qw( ... ); as the common way to export things from a module does not break any code and (hopefully) gets rid of needless inheriting from Exporter.

      In a second step, the way how callbacks are called from Exporter could be changed. Currently, Exporter provides default callbacks as methods in Exporter.pm, so modules inheriting from Exporter don't have to implement them, and $pkg->someCallback() from inside Exporter "just works". (Unless, of course, you accidentally inherited the callback from somewhere else.) In a world where modules don't inherit from Exporter and callbacks are functions, this trick does not work. So Exporter has to check explicitly if the function is actually implemented in the exporting module, and use fallback code if not. That's not very hard, Exporter is already full of namespace checking and manipulation code, adding one or two more checks should not hurt.

      To make that world compatible with our world (i.e. to avoid breaking code), the callbacks need to be called as if they were methods, i.e. with the name of the exporting module as first parameter. And to support even those cases where callbacks are inherited from third party modules, the callbacks actually have to be called as methods.

      Using export_fail as an example, the current code in Exporter/Heavy.pm is:

      @failed = $pkg->export_fail(@failed);

      The default callback is implemented in Exporter.pm as:

      sub export_fail { my $self = shift; @_; }

      Calling the default callback does not change @failed at all. So if the module does not implement or inherit export_fail, just leave @failed unchanged. A simple-minded change would look like this:

      @failed=$pkg->export_fail(@failed) if $pkg->can('export_fail');

      Of course, can has its own nasty surprises. Also, this code still allows inherited callbacks. But with this little change, exporting modules don't have to inherit from Exporter if they want to use the export_fail callback.

      A little bit stricter, breaking inherited callbacks, but without nasty can traps:

      @failed=$pkg->export_fail(@failed) if defined &{"${pkg}::export_fail"} +;

      (Still uses the method call syntax, mainly for backwards compatibility.)

      So, this breaks inherited callbacks. How to fix that? Easy, change the exporting module to import those callbacks instead of inheriting them. The package name is still the first callback argument, so the callback code should need no changes.

      The VERSION callback is even easier, because the default is in UNIVERSAL, not in Exporter. No change needed.

      export_to_level() is a little bit tricky, because - according to the Exporter documentation - it should be called as ExportingModule->export_to_level($level,@_) from ExportingModule::import(). Rewrite that to Exporter::export_to_level('ExportingModule',$level,@_) or Exporter::export_to_level(__PACKAGE__,$level,@_) for the same result. Or, even shorter: Exporter::export_to_level($_[0],$level,@_).

      Now look at the Exporter documentation:

      The export_to_level method looks like:

      MyPackage->export_to_level( $where_to_export, $package, @what_to_export );

      where $where_to_export is an integer telling how far up the calling stack to export your symbols, and @what_to_export is an array telling what symbols *to* export (usually this is @_ ). The $package argument is currently unused.

      (Emphasis mine)

      This is completely ridiculous! The name of the exporting module is passed twice to export_to_level, once via the class method call as implicit first argument, and once as the first element of @_ from ExportingModule::import(), also known as $package parameter.

      Exporter::export_to_level() is just a goto trampoline for Exporter::Heavy::heavy_export_to_level. And that method knows about that nonsense, too:

      sub heavy_export_to_level { my $pkg = shift; my $level = shift; (undef) = shift; # XXX redundant arg my $callpkg = caller($level); $pkg->export($callpkg, @_); }

      How to fix that?

      In ExportingModule::import(), just forget about that fake OOP, and call export_to_level as a function: Exporter::export_to_level($level,@_). (heavy_)export_to_level() must no longer pretend to be a class method, so get rid of that first my $pkg = shift;. The $package argument is no longer redundant, it replaces $pkg:

      my $level=shift; my $pkg=shift; my $callpkg=caller($level); $pkg->export($callpkg,@_);

      To make that compatible with the nonsense-OOP variant, where $package is ignored, use the fact that $level must be an integer:

      my ($level,$pkg); if ($_[1]=~/^[0-9]+$/) { # called as method $pkg=shift; $level=shift; (undef)=shift; } else { # called as function $level=shift; $pkg=shift; } my $callpkg=caller($level); $pkg->export($callpkg,@_);

      Then, consider if export really has to be called as a method. Why would a module implement its own export() method and use Exporer? Not inheriting from Exporter breaks $pkg->export(), so that has to be changed to Exporter::export($pkg,$callpkg,@_), or, knowing that Exporter::export() is just a goto trampolin, to Exporter::Heavy::heavy_export($pkg,$callpkg,@_).

      That would break those strange modules that actually inherit from Exporter and have their own export() method. Again, there is a way to handle that. If you want OOP, keep inheriting from Exporter and keep calling export_to_level() as a method. If not, call export_to_level() as a function:

      my ($level,$pkg,$callpkg); if ($_[1]=~/^[0-9]+$/) { # called as method $pkg=shift; $level=shift; (undef)=shift; $callpkg=caller($level); $pkg->export($callpkg,@_); } else { # called as function $level=shift; $pkg=shift; $callpkg=caller($level); heavy_export($pkg,$callpkg,@_); }

      Have a look at the Exporter documentation. The export() method is documented nowhere, not even as a callback. It's a private method. No other module should implement an export() method when inheriting from Exporter. export() doesn't have to be a method. So, get rid of the method call:

      my ($level,$pkg); if ($_[1]=~/^[0-9]+$/) { # called as method $pkg=shift; $level=shift; (undef)=shift; } else { # called as function $level=shift; $pkg=shift; } my $callpkg=caller($level); heavy_export($pkg,$callpkg,@_);

      Note that Exporter::import() already calls export() as a function, not as a method:

      my $pkg = shift; my $callpkg = caller($ExportLevel); # ... return export $pkg, $callpkg, @_ if $Verbose or $Debug or $fail && @$fail > 1; # ... return export $pkg, $callpkg, ($args ? @_ : ()) if $heavy; # ...

      So if code relies on export() being called as a callback method from within Exporter, it (a) uses undocumented features and (b) is likely to be already broken by the current version of Exporter.

      I've searched for such modules on my computers, the only one that has its own export() method and inherits from Exporter is Time::Piece. But it also has its own import() method that only calls the export() method, so export() is never called from within Exporter there. False positive.

      Alexander

      --
      Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)
Re^3: Advice on style
by Anonymous Monk on Nov 26, 2010 at 10:36 UTC
    Oh wow, that merits a weekly announcement in perlnews

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://873817]
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others chilling in the Monastery: (5)
As of 2015-09-05 15:24 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My preferred temperature scale is:










    Results (156 votes), past polls