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

Avoid stringy eval #20

Merged
merged 3 commits into from Mar 24, 2018

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Mar 24, 2018

Usually, when one wants to check for availability of a given module, it is better to use Class::Load than to call use within a string evaluation. Hence the motivation for this pull request. The main thing that I think you should check is the commit I made to replace stringy eval in the spider tests: I altered the existing behaviour slightly in that, if Dezi::Aggregator::Spider is not present, then the tests are skipped. The match for the error message has been left out in the new version of the code. I think this is ok (it wasn't 100% clear to me what the extra match was achieving, other than providing information about which module was required for the spider tests); if this change isn't ok, then I don't have any problems with leaving it out. Alternatively, if you'd like some particular kind of extra functionality here, just let me know and I can update the PR as necessary.

paultcochrane added some commits Mar 24, 2018

Avoid stringy eval perlcritic warning
... because the "unsafe" version of `eval` is exactly what should be
used in this situation, hence this change just avoids a false positive
from being generated in perlcritic.
@karpet

This comment has been minimized.

Owner

karpet commented Mar 24, 2018

Thanks - I think the POD tests were just boilerplate so I can't take the blame for those. The other ones are all mine.

@karpet karpet merged commit c2d87a0 into karpet:master Mar 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
if ( $@ && $@ =~ m/([\w:]+)/ ) {
skip "$1 required for spider test: $@", 3;
}
try_load_class("Dezi::Aggregator::Spider")

This comment has been minimized.

@karpet

karpet Mar 24, 2018

Owner

This was actually to figure out what the missing dependency was - something that Dezi::Aggregator::Spider was "use"-ing. But this works too.

This comment has been minimized.

@paultcochrane

paultcochrane Mar 24, 2018

Contributor

Hrm, that's what I thought, but I tried uninstalling a module used by D::A::S to see if I could trigger the regexp part required for the skip, but the test would die with a compile time error and wasn't simply skipping the tests, so the eval didn't seem to be doing what it was intended to do. Thanks for clearing that one up for me :-)

@paultcochrane paultcochrane deleted the paultcochrane:pr/avoid-stringy-eval branch Mar 24, 2018

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