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

Add CHI support with options validation #6

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@jmaslak

jmaslak commented May 28, 2016

Howdy, I got assigned your module as part of the CPAN Pull Challenge ( http://neilb.org/2015/12/30/cpan-prc-2016.html ).

I thought support for more generic caching (CHI and similar objects) would be a good addition - along with support for validating that the options weren't changed between cache requests (at least one bug report in RT concerned that) without at least invalidating the cached version.

Basically if a CHI-compatible (implements get() and set()) object is passed as a "cache" option, it's get() and set() are used to cache parsed XML.

Because I didn't want to break old code, only code using these cache objects (rather than the built-in storable, memory, and memorycopy) will do the option validation checking. I thought correctness was more important than speed, even in caching code, so if the options don't match, I don't return the cached parse results. If strictmode is set, it will die in this case, otherwise it will return a warning (assuming warnings are on). I've also of course provided some tests for this.

I also added a VIM modeline to the bottom of Simple.pm, since that might help others ensure they use the same tab settings.

I'm glad to refactor/change anything that I did, based on your thoughts and feedback.

@grantm

This comment has been minimized.

Owner

grantm commented May 31, 2016

Hi Joel

I really appreciate the effort you have put into this but I do wish you'd discussed your plans with me first. I have taken a fairly hard line that I won't accept any pull requests for new features in XML::Simple. The module has too many features already which makes the documentation overwhelming. This in turn means that people don't read the documentation and many of them just email me directly with their problems. I don't know what the solution is but it's not "add more features".

You might like to consider how you would implement your CHI layer as a separate module. I don't make any claim on the XML::Simple::* namespace so you're welcome to use XML::Simple::CHI. If such a module inherited from XML::Simple, it could override methods to add the CHI integration. I would consider a pull request for small changes that made it easier to implement a derived class - for example adding a determine_cache_scheme() method that only supports the existing functionality, but which your module could override to add the CHI-related stuff.

Let me know if you want to try that approach.

Grant

@grantm grantm closed this Mar 18, 2018

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