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

ISHIGAKI/Test-Classy-0.09.tar.gz fails with _071 #480

Closed
andk opened this issue Nov 3, 2014 · 9 comments
Closed

ISHIGAKI/Test-Classy-0.09.tar.gz fails with _071 #480

andk opened this issue Nov 3, 2014 · 9 comments

Comments

@andk
Copy link

andk commented Nov 3, 2014

Sample fail report:

http://www.cpantesters.org/cpan/report/47592595

The report was written with _070 but I just saw a similar result with _071, both with 5.21.5 and 5.20.1

@exodist
Copy link
Member

exodist commented Nov 3, 2014

This module cannot be fixed on our end. Here is why:

  1. Instead of using Test::More->import or Test::More->export_to_level it instead reads @Test::More::EXPORT and manually aliases subs named therin. If it used one of the above interfaces it would be supported as those are maintainable. The system he uses relies on the underlying implementation.

  2. He uses direct hash access into the Test::Builder object to get values that are no longer tracked in Test::Builder itself. These values both had public interfaces (methods) he could have used to access the same values, these public interfaces are still supported, that is not possible in the hash itself.

If this were a critical module that a lot depended on (IE Moose) I would consider extreme measures such as tied hashes or other such magic to try to make it work. But only 3 modules are known on cpan to use it, and all 3 are by the same author.

I would be happy to help this author patch their code to work with the alphas the root of the issue is that he ignored public interfaces and dove directly into internals despite the interfaces being available.

This bug should be reported on the module, not here.

@exodist exodist closed this as completed Nov 3, 2014
@andk
Copy link
Author

andk commented Nov 4, 2014

@charsbar
Copy link

charsbar commented Nov 4, 2014

@exodist, I'd like to know how expensive the cost is to keep @test::More::EXPORT in the new Test::More. Right now grep.cpan.me seems down, but IIRC this is not only module that use @test::More::EXPORT. You mentioned Test::More->import and Test::More->export_to_level, but the former is to select what to import into the current package (not to help export from there), and the latter is not explicitly written in the pod of Test::More as it's actually a feature of Exporter. If you can support export_to_level, why not @export, which is also a well-established interface written in Exporter's pod and other exporter-like modules support?

I've also used @test::More::EXPORT for applications in the wild to write a simple Test::More wrapper to add arbitrary testing functions. Sad if new Test::More breaks backward compatibility at this level.

p.s. I agree with 2), and anyway will fix mine (or deprecate it as I myself stopped using it).

@karenetheridge
Copy link
Member

On Mon, Nov 03, 2014 at 11:07:12PM -0800, Kenichi Ishigaki wrote:

I've also used @test::More::EXPORT for applications in the wild to write a simple Test::More wrapper to add arbitrary testing functions. Sad if new Test::More breaks backward compatibility at this level.

I would consider the existence of an import method, and the arguments it
accepts, as part of the public API, but not generally the fact that it has
an @export variable or other implementation details. I think this is why
Import::Into was written? :)

@exodist
Copy link
Member

exodist commented Nov 4, 2014

@test::More::EXPORT still exists. Too many people use it to get the list of exports for me to remove it. What has changed is that not all subs listed in it exist in the Test::More namespace, some of them are generated exports. Also the variable is not used by import(), so modifying it has no effect.

The module that broke tries to grab the subs from the namespace using typeglobs directly, since some are generated it fails.

@exodist
Copy link
Member

exodist commented Nov 4, 2014

There is also an Test::More->export_to(namespace) method on Test::More now.

@charsbar
Copy link

charsbar commented Nov 4, 2014

@exodist Thanks for the clarification. That's surely my issue, and I'm glad to know @test::More::EXPORT still exists.

@exodist
Copy link
Member

exodist commented Nov 4, 2014

@charsbar my offer still stands to assist you if you have any problems with the alphas, or with making your code work with both versions.

@charsbar
Copy link

charsbar commented Nov 4, 2014

@exodist, thank you :)

@exodist exodist modified the milestone: Stabilize alphas Nov 22, 2014
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

5 participants