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

Why is a custom exporter needed? #73

Open
rjbs opened this issue Dec 8, 2015 · 5 comments
Open

Why is a custom exporter needed? #73

rjbs opened this issue Dec 8, 2015 · 5 comments

Comments

@rjbs
Copy link

rjbs commented Dec 8, 2015

I've heard a lot of questions about whether a custom exporter makes sense. We should discuss this as we go forward.

@exodist
Copy link
Member

exodist commented Dec 8, 2015

I believe the main offender that prompted this issue was the magical import() method events used to have, but that has since been corrected.

On the other hand, the topic of this ticket seems to be about Test::Stream::Exporter. I suspect the topic of the ticket is not actually correct, but in case it is I will switch gears and address that:

Test::Stream::Exporter is used by any modules that need to export functions. This module provides some features I felt were necessary to achieve Test::Stream.

  • The ability to export subs that are not named package subs.
  • The ability to look at an exporter and get information about its exports.
  • The way it works with Test::Stream::HashBase which is the tool that avoids needing to hand-write constructors and accessors.

This last one is also important for performance, using HashBase showed notable, and critical speed improvements that allowed me to achieve performance required by the punch-list.

Some of these features are now available in Exporter.pm, but not all, and not as far back as 5.8.1.

@haarg
Copy link
Member

haarg commented Dec 9, 2015

I looked at how imports are used currently in Test::Stream.

From what I can see, the main reason Test::Stream::Exporter seems to exist is to export unnamed subs from the source package. And those functions are unnamed because they are actually custom import functions.

Test::Stream::Capabilities has a custom import function which serves no purpose. I suspect it primarily exists because of earlier versions of the module that did more work. But currently, the module would be better off with some use constant and Exporter.

I still strongly disagree with the UI of Test::Stream's import. The fact that it is optional does not sway my opinion at all. If use Test::Stream -Classic; is equivalent to use Test::Stream::Bundle::Classic;, the special syntax only serves to confuse. Making it optional does not reduce that confusion.

@exodist
Copy link
Member

exodist commented Dec 9, 2015

I agree on Test::Stream::Capabilities, as it is now the custom import is overkill. iI would like to see a seperate ticket for that, and I will fix it, or accept tickets that do.

I disagree on the Test::Stream UI (predictably), but I think that also deserves its own ticket and discussion seperate from here.

@exodist
Copy link
Member

exodist commented Dec 9, 2015

Issue #75 was opened for the Test::Stream::Capabilities issue. Issue #76 was opened for the Test::Stream UI discussion.

@rjbs
Copy link
Author

rjbs commented Dec 11, 2015

Update after weekly status meeting. @exodist reports that the "split" branch, in which the essential elements are split from the toolkit, only Exporter.pm is used.

@haarg: 👍?

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

No branches or pull requests

3 participants