Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] use Importer to enhance Exporter in blead #15151

Open
p5pRT opened this issue Jan 27, 2016 · 8 comments
Open

[PATCH] use Importer to enhance Exporter in blead #15151

p5pRT opened this issue Jan 27, 2016 · 8 comments
Labels

Comments

@p5pRT
Copy link

p5pRT commented Jan 27, 2016

Migrated from rt.perl.org#127384 (status was 'open')

Searchable as RT127384$

@p5pRT
Copy link
Author

p5pRT commented Jan 27, 2016

From @exodist

Attached is a patch to perl blead. This patch brings in the Importer
distribution, and updates Exporter.pm to use it for any feature beyond
basic sub exporting. This also updates a couple other files that needed to
be updated due to the changes.

Key things to note​:

* Nothing existing needs to change.
* Exporter will still work for everything it has always worked for
* Nothing is required to start using Importer
* This introduces the { -as => 'new_name' } syntax discussed in another
thread
* This puts it on the consumer of an exporter to list an Importer version
in their deps if they depend on a specific feature not available in old
exporters.

-Chad

@p5pRT
Copy link
Author

p5pRT commented Jan 27, 2016

@p5pRT
Copy link
Author

p5pRT commented Jan 27, 2016

From @karenetheridge

Since this is adding a new module to core, Module​::CoreList also needs to
be updated. (I would have expected a Porting test to fail? Is this possible
to add? BinGOs?)

I'm presuming that the discussion of whether to adding Importer to core has
already happened, and I missed it.

On Tue, Jan 26, 2016 at 8​:07 PM, Chad Granum <perlbug-followup@​perl.org>
wrote​:

# New Ticket Created by Chad Granum
# Please include the string​: [perl #127384]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127384 >

Attached is a patch to perl blead. This patch brings in the Importer
distribution, and updates Exporter.pm to use it for any feature beyond
basic sub exporting. This also updates a couple other files that needed to
be updated due to the changes.

Key things to note​:

* Nothing existing needs to change.
* Exporter will still work for everything it has always worked for
* Nothing is required to start using Importer
* This introduces the { -as => 'new_name' } syntax discussed in another
thread
* This puts it on the consumer of an exporter to list an Importer version
in their deps if they depend on a specific feature not available in old
exporters.

-Chad

@p5pRT
Copy link
Author

p5pRT commented Jan 27, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jan 27, 2016

From @exodist

Strange, no CoreList tests failed. So yeah, the patch will need that added.

There has not been a discussion about adding Importer to core, I expected that to happen in the comments on this ticket.

What has happened is a discussion about improving Exporter.pm, that discussion also produced concerns that consumers of exports will have the burden of depending on a specific exporter version, having the Importer module is intended to solve that secondary problem.

@p5pRT
Copy link
Author

p5pRT commented Feb 7, 2016

From @tonycoz

On Tue Jan 26 20​:07​:39 2016, exodist7@​gmail.com wrote​:

Attached is a patch to perl blead. This patch brings in the Importer
distribution, and updates Exporter.pm to use it for any feature beyond
basic sub exporting. This also updates a couple other files that needed to
be updated due to the changes.

Key things to note​:

* Nothing existing needs to change.
* Exporter will still work for everything it has always worked for
* Nothing is required to start using Importer
* This introduces the { -as => 'new_name' } syntax discussed in another
thread
* This puts it on the consumer of an exporter to list an Importer version
in their deps if they depend on a specific feature not available in old
exporters.

This doesn't really solve the Exporter new features problem.

It assumes that a module uses the Exporter package variables (@​EXPORT etc), but a module might choose to supply extra export features by switching to Exporter​::Tidy or Sub​::Exporter instead, neither of which use the Exporter package variables*.

The only way either new Exporter features or Importer are safe to use is for the upstream package author to promise that such features are available (and they implement them either by a dependency on the newer Exporter or by setting the variables Importer needs).

Tony

* from a quick look at the documentation

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2016

From @rjbs

* Chad Granum <perlbug-followup@​perl.org> [2016-01-26T23​:07​:40]

Attached is a patch to perl blead. This patch brings in the Importer
distribution, and updates Exporter.pm to use it for any feature beyond
basic sub exporting. This also updates a couple other files that needed to
be updated due to the changes.

I don't think I can get behind this.

It's "I came up with an interface a couple weeks ago and now want to tie one of
the most depended-on libraries in the CPAN ecosystem to it." There isn't any
explanation or justification as far as I see in this thread. Did I miss that
happening somewhere else? If so, please help me out with a cross-reference.

The docs on Importer.pm only mention that it has renaming as a thing, but it's
not clear whether Importer.pm alone makes -as work universally. Is this patch
required to Exporter to make Importer's renaming work with it? If so, then as
Tony said, the main objection to the original change -- there's no way for the
client code to ensure that this will work -- seems unanswered. Would the
user be expected, forever, to write "use Importer 0.123 ..." because 0.123 is
the first one that forces Exporter to be new enough to work?

This patch seems to me to introduce a piece of machinery that may be useful,
but is neither proven nor necessary.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Feb 8, 2016

From @exodist

I think I did fail to communicate adequately about this, and that is on me.

*(Skip to the end if you do not want the explanation, but want some other
proposals)*

- After proposing the addition of '-as' to Exporter.pm there was a lot of
concern over the fact that a provider uses exporter, but a consumer depends
on the features of exporter. This leads to a situation where a module that
consumes exports will need to list an Exporter.pm version in their dep
list. This is also a problem because if we add the feature to exporter.pm
we effectively inject a new feature into all modules that use Exporter.pm,
which means if they Choose to switch to a different exporter they may break
their downstream consumers. This entire situation seems pretty crazy.

- Someone pointed out that @​ISA and base/parent seem to be a similar
*thing* but don't have the same problem because it is 'use base
"Base​::Class";'. I then decided to try to write a tool for imports/exports
that followed their example. This is where the idea for Importer came from.

- I did not tackle Importer as something new, but instead as a refactor of
Exporter.pm. In the process I added new features and removed a *LOT* of
cruft. This is also good because Importer works on any module that uses
Exporter.pm.

- I used the Importer namespace for several reasons​:
  - No sane way to work the approach directly into Exporter.pm, 'use
Exporter "Foo​::Bar"' would be a breaking change
  - Many consumers of Exporter.pm subclass it, so adding methods to
Exporter.pm means adding them to innumerable other modules
  - I do not have permissions on Exporter.pm
  - I was able to put it on cpan, which is all I initially wanted.

Once I had it finished to my specification I decided I needed to be SURE it
was 100% Exporter.pm compatible. I decided the best way to do this was to
write an Exporter.pm patch that used Importer under the hood. Then I could
run all the blead tests, and smoke a bunch of cpan to see what broke. This
was very useful and did in fact find bugs, all of which have been fixed.

Once I had this I realized that the new Exporter.pm version was notably
faster and more maintainable than the old. I decided to go ahead and submit
the patch to blead to see if people would be interested​:

- It is faster
- It is cleaner and easier to maintain
- Completely backwards compatible
- It has a the desired new features, including '-as' for renaming
- It addresses the 'interface' problem by letting you 'use Importer
"Foo​::Bar"' in new code
- Making them both share a codebase means neither one will grow out of
sync with the other.

Now, to answer a specific question​: No, the patch to Exporter.pm is not
necessary for Importer to work, they can co-exist without this patch.

I am not going to push to hard on this. I see a lot of benefit, and the
work is done if you want it. I am also perfectly happy just having
Importer.pm on cpan.

*Other Options​:*

Take the refactor work form Importer.pm, but put it mostly into
Carp​::Heavy. This can get the de-crazy benefit, and the new feature benefit
without adding Importer.pm to core. But it does mean bugfixes will need to
go to 2 places.

I can also strip out the extra features Importer.pm has if the problem is
that people don't want features in Importer.pm

Import can be core'd without changing Exporter.pm, but it might as well
just stay on cpan, it doesn't need to be core.

-Chad

On Sun, Feb 7, 2016 at 7​:10 PM, Ricardo Signes <perl.p5p@​rjbs.manxome.org>
wrote​:

* Chad Granum <perlbug-followup@​perl.org> [2016-01-26T23​:07​:40]

Attached is a patch to perl blead. This patch brings in the Importer
distribution, and updates Exporter.pm to use it for any feature beyond
basic sub exporting. This also updates a couple other files that needed
to
be updated due to the changes.

I don't think I can get behind this.

It's "I came up with an interface a couple weeks ago and now want to tie
one of
the most depended-on libraries in the CPAN ecosystem to it." There isn't
any
explanation or justification as far as I see in this thread. Did I miss
that
happening somewhere else? If so, please help me out with a
cross-reference.

The docs on Importer.pm only mention that it has renaming as a thing, but
it's
not clear whether Importer.pm alone makes -as work universally. Is this
patch
required to Exporter to make Importer's renaming work with it? If so,
then as
Tony said, the main objection to the original change -- there's no way for
the
client code to ensure that this will work -- seems unanswered. Would the
user be expected, forever, to write "use Importer 0.123 ..." because 0.123
is
the first one that forces Exporter to be new enough to work?

This patch seems to me to introduce a piece of machinery that may be
useful,
but is neither proven nor necessary.

--
rjbs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants