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

Minor changes #7

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@fluca1978

fluca1978 commented Jan 4, 2015

WHile working for the 2015 CPAN Pull Request Challenge, I looked at the excellent Test::Pod module and found a few things that, in my opinion, could be imroved.
Besides a few syntactic only things, most notably changes are related to the adoption of the Exporter module (instead of an hand written import routine) and a few enhancement in the internal method _is_perl, that is now coherent with the documentation.

fluca1978 added some commits Jan 3, 2015

Removed a mixed use of low and high precedence "and" operators.
Removed also an exra spaces in a method call parentheses.
Merged three regular expression in a single line.
The regular expressions search for a Perl like file inspecting the common extensions in the private method _is_perl.
Fixed a test in the internal _is_perl method to test for a (suspected…
…) Perl file.

The POD documentation stated that a Perl file is, among the others, "any file that ends in .bat and has a first line with --*-Perl-*--", however the _is_perl method was testing only for the latter condition, skipping the .bat part.
Now the _is_perl method tests that the file has either a shee-bang or the right well-known Perl suffix OR ends with .bat and has the Perl comment line as first.
The test for the well known Perl extension has been pushed to the end of the method, so that if a file cannot be read (i.e., open fails) the file is not considered to be a good Perl file (e.g., it is not readable).
Removed the old-style import manual subroutine.
Added the usage of the Exporter module and placed the @export method list for exporting.
@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 5, 2015

Thanks. Some comments:

  • Enabled "warnings" and placed an explicit return statement into the module import method.

Warnings are not necessary in such a stable module. No harm, really.

  • Removed a mixed use of low and high precedence "and" operators.

Seems reasonable.

  • Merged three regular expression in a single line.

Would be better written as /[.](?:PL|p(?:[lm]|od)|t)$/

  • Fixed a test in the internal _is_perl method to test for a (suspected) Perl file.

I think it should look at the file name and return 1 for a match before opening the file and reading the first line, as it did before. Silly to read in a line if you never examine it.

Also, I see you added a check for .bat. I don't know if that's necessary, but it would be better written as /[.]bat$/i.

Also, you don't need all the parentheses in that expression.

  • Removed the old-style import manual subroutine.

Adding the unnecessary overhead of the Exporter module and removing a call to exported_to.

theory commented Jan 5, 2015

Thanks. Some comments:

  • Enabled "warnings" and placed an explicit return statement into the module import method.

Warnings are not necessary in such a stable module. No harm, really.

  • Removed a mixed use of low and high precedence "and" operators.

Seems reasonable.

  • Merged three regular expression in a single line.

Would be better written as /[.](?:PL|p(?:[lm]|od)|t)$/

  • Fixed a test in the internal _is_perl method to test for a (suspected) Perl file.

I think it should look at the file name and return 1 for a match before opening the file and reading the first line, as it did before. Silly to read in a line if you never examine it.

Also, I see you added a check for .bat. I don't know if that's necessary, but it would be better written as /[.]bat$/i.

Also, you don't need all the parentheses in that expression.

  • Removed the old-style import manual subroutine.

Adding the unnecessary overhead of the Exporter module and removing a call to exported_to.

fluca1978 added some commits Jan 5, 2015

Improved the _is_perl internal method following the suggestions of Da…
…vid, see #7

As suggested, the method should accept any file with a good suffix before trying to open it, we are sure it is a file from the finding subroutine.
All regular expressions are now using non-capturing groups, since there is no back reference used at all within any scope. All regular expressions dealing with file name (extensions) use the /x capability for better readibility.
If a file has not a well know suffix it will be tested against a shee bang in the first line or, if it has a .bat suffix, it will be tested againt a --*-Perl-*-- first line.
@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 Jan 5, 2015

I've "restored" the original logic of _is_perl, so that it checks for well known suffixes before opening the file. The rationale before my changes was that letting the file opening will test for readibility, that is not checked before calling pod_file_ok. It is not clear to me if having a not-readable file should be reported as an invalid pod file or skipped at all from the test.
In the meantime I've "fixed" all the regular expressions using your suggestions.

With regard to the usage of Exporter, it seems to me it produces less code for mere mortals (and removes the "no strict refs" that is something I tend to avoid). Is the usage of Exporter here so heavy?

fluca1978 commented Jan 5, 2015

I've "restored" the original logic of _is_perl, so that it checks for well known suffixes before opening the file. The rationale before my changes was that letting the file opening will test for readibility, that is not checked before calling pod_file_ok. It is not clear to me if having a not-readable file should be reported as an invalid pod file or skipped at all from the test.
In the meantime I've "fixed" all the regular expressions using your suggestions.

With regard to the usage of Exporter, it seems to me it produces less code for mere mortals (and removes the "no strict refs" that is something I tend to avoid). Is the usage of Exporter here so heavy?

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 5, 2015

@rjbs points out that the existing import also allows a plan to be passed to it. There may well be code in the wild that depends on that, so I think we should leave it and not switch to Exporter.

theory commented Jan 5, 2015

@rjbs points out that the existing import also allows a plan to be passed to it. There may well be code in the wild that depends on that, so I think we should leave it and not switch to Exporter.

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 5, 2015

In fact, it is used in the wild. (Thanks #rjbs!)

theory commented Jan 5, 2015

In fact, it is used in the wild. (Thanks #rjbs!)

@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 Jan 6, 2015

@rjbs points out that the existing import also allows a plan to be passed to it.

Sorry for that silly error, I didn't notice it because all tests were passing. Should we add a forwarding method to get out plan information on Test::Pod from inner Test::Builder? This way we could also add a test to check that the plan (if specified) is correct.

Besides, do you believe there is something on this pull request that can be cherry picked? Otherwise please drop it at all.

fluca1978 commented Jan 6, 2015

@rjbs points out that the existing import also allows a plan to be passed to it.

Sorry for that silly error, I didn't notice it because all tests were passing. Should we add a forwarding method to get out plan information on Test::Pod from inner Test::Builder? This way we could also add a test to check that the plan (if specified) is correct.

Besides, do you believe there is something on this pull request that can be cherry picked? Otherwise please drop it at all.

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 6, 2015

Should we add a forwarding method to get out plan information on Test::Pod from inner Test::Builder? This way we could also add a test to check that the plan (if specified) is correct.

I don't understand the question.

theory commented Jan 6, 2015

Should we add a forwarding method to get out plan information on Test::Pod from inner Test::Builder? This way we could also add a test to check that the plan (if specified) is correct.

I don't understand the question.

@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 Jan 6, 2015

The original import method has a final $Test->plan(@_);' call. However seems to me no one test within the t directory will test for an effective plan, so maybe we should add a method to Test::Pod that allows us to retrieve the Test::Builder::has_plan value, so that we can also test ourselves for the plan to be set.
Of course the plan is working, what am I saying is that my stupid error was also driven by the fact that the plan is not tested at all within the module.

fluca1978 commented Jan 6, 2015

The original import method has a final $Test->plan(@_);' call. However seems to me no one test within the t directory will test for an effective plan, so maybe we should add a method to Test::Pod that allows us to retrieve the Test::Builder::has_plan value, so that we can also test ourselves for the plan to be set.
Of course the plan is working, what am I saying is that my stupid error was also driven by the fact that the plan is not tested at all within the module.

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 6, 2015

I'm still not following. The point is not to retrieve the plan, but to declare it. Here's an example:

use Test::Pod tests => 1;

theory commented Jan 6, 2015

I'm still not following. The point is not to retrieve the plan, but to declare it. Here's an example:

use Test::Pod tests => 1;
@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 Jan 6, 2015

My point is that when I removed the import method, and therefore the Test::Builder->plan call, I was unable to see the error because no Test::Pod tests were reporting it. If we have a test that tries to use the module with a few plans we can test that the importing-with-plan functionality will always work.
But to be able to test it we have to forward Test::Builder->has_plan method.
Hope it is clear and useful.

fluca1978 commented Jan 6, 2015

My point is that when I removed the import method, and therefore the Test::Builder->plan call, I was unable to see the error because no Test::Pod tests were reporting it. If we have a test that tries to use the module with a few plans we can test that the importing-with-plan functionality will always work.
But to be able to test it we have to forward Test::Builder->has_plan method.
Hope it is clear and useful.

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 7, 2015

Oh, you want to be able to see what the plan gets set to from tests? Since Test::Builder is a singleton, I'm pretty sure you can just get that from Test::Builder->new->expected_tests.

theory commented Jan 7, 2015

Oh, you want to be able to see what the plan gets set to from tests? Since Test::Builder is a singleton, I'm pretty sure you can just get that from Test::Builder->new->expected_tests.

fluca1978 added a commit to fluca1978/test-pod that referenced this pull request Jan 7, 2015

Improved the _is_perl internal method following the suggestions of Da…
…vid, see perl-pod#7

As suggested, the method should accept any file with a good suffix before trying to open it, we are sure it is a file from the finding subroutine.
All regular expressions are now using non-capturing groups, since there is no back reference used at all within any scope. All regular expressions dealing with file name (extensions) use the /x capability for better readibility.
If a file has not a well know suffix it will be tested against a shee
bang in the first line or, if it has a .bat suffix, it will be tested
against a --*-Perl-*-- first line.
Note that the MS Windows batch file extension is checked with the
ignore-case flag, while the *nix-like well know suffixes are not, since
it make sense to distinguish good cases .pl and .PL but not .Pl.
@fluca1978

This comment has been minimized.

Show comment
Hide comment
@fluca1978

fluca1978 Jan 7, 2015

Can you please close this pull request?
I'm going to create a new one with less changes and the correct (source) import behavior.

fluca1978 commented Jan 7, 2015

Can you please close this pull request?
I'm going to create a new one with less changes and the correct (source) import behavior.

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Jan 7, 2015

Awesome. Thanks for taking this on!

theory commented Jan 7, 2015

Awesome. Thanks for taking this on!

@theory theory closed this Jan 7, 2015

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