search

rjbs / Pod-Weaver

recombine pod documents into awesomer pod documents
26 stars 28 forks source link

[minor] /m modifier in Role::StringFromComment might not be required #33

Closed kentfredric closed 9 years ago

kentfredric commented 9 years ago

https://github.com/rjbs/Pod-Weaver/blob/d62c6f8a6bf5067598648fde6b3a89973d34320a/lib/Pod/Weaver/Role/StringFromComment.pm#L49

/m has notable portability problems before 5.10.0 where the /m modifier was lost. ( Stupidly, it is documented in the 5.10.1 delta as being neglected to be mentioned in the 5.10.0 delta ... but nobody updated the 5.10.0 delta to reflect that )

Looking at the archaeology, it looks like the reason /m was required in the first place is once upon a time, the comment extraction was performed on the whole document in serialised form, and was later refined to matching simply comment nodes:

https://metacpan.org/diff/file?target=RJBS/Pod-Weaver-3.101633/lib/Pod/Weaver/Section/Name.pm&source=RJBS/Pod-Weaver-3.101631/lib/Pod/Weaver/Section/Name.pm

https://github.com/rjbs/Pod-Weaver/commit/67d1ae7af8ba1169f776478a619a36a9ce80fdfc

I don't believe PPI::Token::Comment nodes can span multiple lines, so the /m match may not be required.

rjbs commented 9 years ago

I think I'd want to see proof that it can't span multiple lines. Unless you're really keen for this change, don't trouble yourself. I'll get to it eventually.

rjbs commented 9 years ago

I'm closing this. I expect I'll just be requiring 5.10 in the nearish future.

kentfredric commented 9 years ago

An alternative approach is you could just replace $ in the regex with (?=\n|\z), which I believe is the pre-5.10 portable way of having $ match what you expect without the /m modifier.