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

Reduce POD warnings and errors #58

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Jun 12, 2015

These changes fix all errors and most warnings thrown by podchecker.

Comments and feedback are most certainly welcome.

paultcochrane added some commits Jun 12, 2015

Add space after =cut statement
This corrects the 'Spurious text after =cut' errors found by podchecker.
Remove empty =over blocks
This removes the 'No items in =over' warning from podchecker
Escape slash char inside POD link
Inside POD links (L<>), one needs to escape special characters such
as / and |.  This change replaces the slash char in the link with
its relevant POD escape: E<sol>.  Admittedly this is hard to read
in the plain text, however the generated link correctly jumps to
its target and podchecker doesn't complain so much.
Replace sectioned links with C<> formatting
Using L<> for sectioned links (e.g. L<perl(1)>) is deprecated.  The
Pod::Checker docs state that "POD hyperlinks may point to POD documents
only."  This change also removes a warning from podchecker.
@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 12, 2015

Member

Any chance of following this with an xt/pod.t test, so that a PR like this is never needed again? If in doubt - steal the one from DBIC.

Thanks!

Member

ribasushi commented Jun 12, 2015

Any chance of following this with an xt/pod.t test, so that a PR like this is never needed again? If in doubt - steal the one from DBIC.

Thanks!

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jun 12, 2015

Contributor

Sure, no worries! I've got a couple of other ideas which would probably fit well with such a test, so let's keep this PR on ice until I've gotten the other code written.

Contributor

paultcochrane commented Jun 12, 2015

Sure, no worries! I've got a couple of other ideas which would probably fit well with such a test, so let's keep this PR on ice until I've gotten the other code written.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 24, 2015

Actually this is incorrect - all podparsers properly understand / in the link-part of a L<>. See also: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082820/lib/DBIx/Class/Manual/DocMap.pod#L70

ribasushi commented on c6d070f Jun 24, 2015

Actually this is incorrect - all podparsers properly understand / in the link-part of a L<>. See also: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082820/lib/DBIx/Class/Manual/DocMap.pod#L70

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jun 25, 2015

Owner

Actually, line 70 isn't the problem in DocMap.pod, since the slash is part of specifying a link to a section. The problem is with a slash being used as part of a section heading, e.g. GETTING HELP/SUPPORT (line 66). In that case the slash needs to be escaped since it is part of the heading and not a delimiter of the link. By the way, podchecker gives a warning for this issue in DocMap.pod as well:

** WARNING: node 'GETTING HELP/SUPPORT' contains non-escaped | or / at line 66 in file DocMap.pod

This is the same warning as I found in lib/SQL/Translator.pm.

To be honest, I agree with you: all of the pod parsers probably parse this correctly. Nevertheless, I was simply trying to reduce/remove the warnings that podchecker generates. I'm not worried if you don't want to merge the change, it is intended to be helpful, but if it isn't, then I've just had bad luck :-)

BTW: as far as an xt test for this goes: do you just want the errors to be reported? Or should the warnings be flagged as well?

Owner

paultcochrane replied Jun 25, 2015

Actually, line 70 isn't the problem in DocMap.pod, since the slash is part of specifying a link to a section. The problem is with a slash being used as part of a section heading, e.g. GETTING HELP/SUPPORT (line 66). In that case the slash needs to be escaped since it is part of the heading and not a delimiter of the link. By the way, podchecker gives a warning for this issue in DocMap.pod as well:

** WARNING: node 'GETTING HELP/SUPPORT' contains non-escaped | or / at line 66 in file DocMap.pod

This is the same warning as I found in lib/SQL/Translator.pm.

To be honest, I agree with you: all of the pod parsers probably parse this correctly. Nevertheless, I was simply trying to reduce/remove the warnings that podchecker generates. I'm not worried if you don't want to merge the change, it is intended to be helpful, but if it isn't, then I've just had bad luck :-)

BTW: as far as an xt test for this goes: do you just want the errors to be reported? Or should the warnings be flagged as well?

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 25, 2015

To be honest, I agree with you: all of the pod parsers probably parse this correctly.

Right, my motivation here is "if it can be made to work in all cases with less thought - we will use that". So I am disinclined to silence this particular warning.

As far as the test goes - perhaps have the .t itself ignore this warning itself, and present all others? The rest of the issues flagged were rather valuable.

ribasushi replied Jun 25, 2015

To be honest, I agree with you: all of the pod parsers probably parse this correctly.

Right, my motivation here is "if it can be made to work in all cases with less thought - we will use that". So I am disinclined to silence this particular warning.

As far as the test goes - perhaps have the .t itself ignore this warning itself, and present all others? The rest of the issues flagged were rather valuable.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 24, 2015

Surely this TODO needs to go? :)

ribasushi commented on lib/SQL/Translator/Filter/Names.pm in eef0c99 Jun 24, 2015

Surely this TODO needs to go? :)

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jun 25, 2015

Owner

Hrm, something weird happened with that patch. Actually, BUGS and TODO need to go, and the text on lines 105 and 106 needs to be under the DESCRIPTION heading. Sorry that this go through! I'll fix up this commit and push the change in a new PR.

Owner

paultcochrane replied Jun 25, 2015

Hrm, something weird happened with that patch. Actually, BUGS and TODO need to go, and the text on lines 105 and 106 needs to be under the DESCRIPTION heading. Sorry that this go through! I'll fix up this commit and push the change in a new PR.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 24, 2015

Member

@paultcochrane In the meantime I have applied 94cbc3d, 4646fca and 5f8797a
as
33d693c, f60d4c6 and 0e9badb respectively.

The remaining 2 commits in this PR will stay on ice until you get back to this.

Member

ribasushi commented Jun 24, 2015

@paultcochrane In the meantime I have applied 94cbc3d, 4646fca and 5f8797a
as
33d693c, f60d4c6 and 0e9badb respectively.

The remaining 2 commits in this PR will stay on ice until you get back to this.

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 25, 2015

Member

As per short IRC conversation this PR will be closed with no further action. The outstanding work is resolved as follows:
c6d070f - The slashes within L<> headings will not be converted to E<> sequences. A future xt/ test will make steps to explicitly ignore this warnings as it is of no practical consequence.
eef0c99 - Seems like a botched partial commit, will be resubmitted as a separate PR

Member

ribasushi commented Jun 25, 2015

As per short IRC conversation this PR will be closed with no further action. The outstanding work is resolved as follows:
c6d070f - The slashes within L<> headings will not be converted to E<> sequences. A future xt/ test will make steps to explicitly ignore this warnings as it is of no practical consequence.
eef0c99 - Seems like a botched partial commit, will be resubmitted as a separate PR

@ribasushi

This comment has been minimized.

Show comment
Hide comment
@ribasushi

ribasushi Jun 25, 2015

Member

@paultcochrane let me know if I missed something ;)

Member

ribasushi commented Jun 25, 2015

@paultcochrane let me know if I missed something ;)

@ribasushi ribasushi closed this Jun 25, 2015

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jun 25, 2015

Contributor

@ribasushi looks good :-)

Contributor

paultcochrane commented Jun 25, 2015

@ribasushi looks good :-)

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