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

Removed unused Test::Base directives. #10

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@fluca1978

fluca1978 commented Dec 21, 2015

Thanks to miyagawa as suggested here
#9 (comment)

Removed unused Test::Base directives.
Thanks to miyagawa as suggested here
<#9 (comment)>
@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 21, 2015

Owner

Thank you for the PR.

Is it necessary to remove auto_include as well? As of the three modules required for testing, Test::Warn is not in core; removing auto_include will force the user to install the module.

cc @miyagawa

Owner

kazuho commented Dec 21, 2015

Thank you for the PR.

Is it necessary to remove auto_include as well? As of the three modules required for testing, Test::Warn is not in core; removing auto_include will force the user to install the module.

cc @miyagawa

@miyagawa

This comment has been minimized.

Show comment
Hide comment
@miyagawa

miyagawa Dec 21, 2015

That is correct. However a) I consider auto_include fundamentally broken (as is everything else in Module::Install) and b) Parallel::Scoreboard already has other CPAN dependencies, and adding one wouldn't make install any more problematic (see below though).

I would recommend you to migrate to Mi(ni)lla so that test_requires would actually be creating a test dependency so that packagers and a user running cpanm --notest would skip the module. But that could be done outside this PR.

miyagawa commented Dec 21, 2015

That is correct. However a) I consider auto_include fundamentally broken (as is everything else in Module::Install) and b) Parallel::Scoreboard already has other CPAN dependencies, and adding one wouldn't make install any more problematic (see below though).

I would recommend you to migrate to Mi(ni)lla so that test_requires would actually be creating a test dependency so that packagers and a user running cpanm --notest would skip the module. But that could be done outside this PR.

kazuho added a commit that referenced this pull request Dec 22, 2015

@kazuho

This comment has been minimized.

Show comment
Hide comment
@kazuho

kazuho Dec 22, 2015

Owner

@miyagawa Thank you for the clarification. Will only remove use_test_base for the time being.

Owner

kazuho commented Dec 22, 2015

@miyagawa Thank you for the clarification. Will only remove use_test_base for the time being.

@kazuho kazuho closed this Dec 22, 2015

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