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

Avoid string eval #12

Merged
merged 2 commits into from Aug 9, 2018

Conversation

Projects
None yet
2 participants
@paultcochrane
Copy link
Contributor

paultcochrane commented Jul 10, 2018

This PR avoids string eval where possible and tells perlcritic not to stress itself where it's necessary to use a string eval. This change helps the code conform to perlcritic severity level 5.

If you want me to change anything, please just let me know and I'll update and resubmit as necessary.

Paul Cochrane added some commits Jul 10, 2018

Paul Cochrane
Quieten perlcritic on necessary string eval
This particular string eval actually does need to be evaluated at
runtime, hence we let perlcritic know that this is a false positive.
@paultcochrane

This comment has been minimized.

Copy link
Contributor Author

paultcochrane commented Jul 10, 2018

While working on this PR I noticed that most of the test files are almost identical to tests in XMLRPC::Lite. Namely, the tests 07-xmlrpc_payload.t, 26-xmlrpc.t, 37-mod_xmlrpc.t are all practically identical to the test files of similar names in XMLRPC::Lite. Is this intentional? It also looks like the tests aren't actually testing XML::Parser::Lite at all (only 27-xmlparserlite.t and Lite.t actually use XML::Parser::Lite). Are these files necessary in this dist? Should I submit a PR to remove them?

@redhotpenguin

This comment has been minimized.

Copy link
Owner

redhotpenguin commented Aug 9, 2018

I'll leave it up to your judgement to remove them. It sounds like they are holdovers from xmlrpc-lite. They were intertwined when both were subsets of SOAP::Lite, and so are likely redundant in this distribution.

@redhotpenguin redhotpenguin merged commit c44ded5 into redhotpenguin:master Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.