-
Notifications
You must be signed in to change notification settings - Fork 3
port to use Pod::Simple #2
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
Comments
Can you explain more as to what the point of such a change would be? |
Pod::Parser has been removed from perl core, so it's an extra unnecessary dependency that needs to be installed. Pod::Parser is at this point only seeing minimal maintenance. Pod::Simple makes it much easier to accurately handle things like formatting codes, which are currently not entirely properly handled by Pod::Coverage. It would also make other future enhancements easier to write. |
Ii's no issue that the dependency is no longer core, as we don't rely on it being so per the Makefile.PL. I'm interested in this formatting codes issue you mention, so like to explore a fix for that. I'd be surprised if a rewrite is the only way to handle it though. |
Having the extra dependency is not strictly an issue, but it's an extra install that is unnecessary, since Pod::Simple is always available. Pod::Coverage currently tries to handle Pod::Parser does not have any encoding support, so it will not properly decode headings with unicode characters. While these are uncommon, they are supported by perl core. There are various Pod::Coverage extensions on CPAN. One of the most commonly used is Pod::Coverage::TrustPod. Adding that feature to Pod::Coverage would be much easier if the parser was using Pod::Simple. Rewriting the parser is not particularly difficult. I've already done it. Integrating it into Pod::Coverage itself would involve some cleanups, and possibly removing extra features (like the Pod::Coverage::TrustPod support). |
As mentioned in #2 Pod::Parser doesn't handle encodings, which may cause some form of failure. Here in order to understand this, and in the attempt to add a failing test that can be fixed with the correct behaviour, we add a simple test fixture UnicodeHeading that refers to U+1F3A9, the Top Hat in a pod heading. At present it doesn't fail, so more work needed.
Apologies for the delay in response, I think several points are getting a little conflated, and I couldn't see a clean solution to all of them. I think the case you're making for the crrect handling of all the formatting entities works, so yes, a fix for that please.
Do you have an example of one of those so that we might make a failing test case? Just a trivial 'throw some unicode in there' didn't fail for me, so I'm interested in what a real-world example would look like. Also could we move that over to the discussion for #4
Are you suggesting that Pod::Coverage could start absorbing the feature sets of other distributions in the ecosystem? |
I'll follow up on the other parts later when I'm at a real computer, but wanted to address this part now:
Partly, yes. The existing extensions already have to do their own pod parsing layered on top of what Pod::Coverage does. Additional extensions then would need to implement that again. And features from the extensions can't be used together unless one extends the other. A lot of this would be easier if the features were options on the same module. And I think using Pod::Simple would also provide a more solid base for extensions to build on even if no features were added. I wanted a new feature for pod coverage and writing it as an extension basically meant rewriting the entire module, at which point the only thing Pod::Coverage itself was doing for me was bringing a dependency on Pod::Parser, which honestly I want gone from the entire ecosystem. |
Would you accept a patch to this module porting it to use Pod::Simple rather Pod::Parser? I have already written the code for this, although as part of a different module. I could work on porting it if this is something you are interested in.
The text was updated successfully, but these errors were encountered: