PPI
PPI copied to clipboard
Perl-Critic
Perl::Critic::Utils should be moved to something like PPI::Helpers
There's lots of useful generic stuff in Perl::Critic::Utils, and various things (e.g. Perl::MinimumVersion) call into it to do its code parsing. I'd like to consider we take on this code into a separate distribution that is more closely associated with core PPI.
Looking at the methods in that package, there's a bunch that should really just be migrated over into some PPI class as a first class method.
Rather than split out a large, mixed and somewhat arbitrary collection of methods into a standalone distribution, we should just absorb the cleanest bits directly.
Starting with the simplest and most obvious ones, we could just make a branch or pull request for each one.
That would give us a chance to discuss if the implementation is high enough for PPI core, and potentially change the method names to something aligned with the naming style of PPI itself.
If we do a good enough job, then we won't need to impose additional dependencies but instead we can remove them, which would be fantastic.
What do you think?
Adam
On 26 March 2014 17:26, Karen Etheridge [email protected] wrote:
There's lots of useful generic stuff in Perl::Critic::Utils, and various things (e.g. Perl::MinimumVersion) call into it to do its code parsing. I'd like to consider we take on this code into a separate distribution that is more closely associated with core PPI.
Reply to this email directly or view it on GitHubhttps://github.com/adamkennedy/PPI/issues/78 .
+1, @adamkennedy . A survey of services that, e.g., Perl::Critic policies ask P::C and PPI for is easy to get via NYTProf. Those plus wheels that the policies reinvent would be a good list of things to consider.
For P::C I find that the majority of time is spent either in the policy or in find(), rather than in the initial PPI parse of the document. (Sorry, no numbers at the moment.) Efficient (and cachable) services from PPI could help runtime on top of the other benefits.
Indeed, although critic already caches heavily. On Mar 26, 2014 8:22 PM, "moregan" [email protected] wrote:
+1, @adamkennedy https://github.com/adamkennedy . A survey of services that, e.g., Perl::Critic policies ask P::C and PPI for is easy to get via NYTProf. Those plus wheels that the policies reinvent would be a good list of things to consider.
For P::C I find that the majority of time is spent either in the policy or in find(), rather than in the initial PPI parse of the document. (Sorry, no numbers at the moment.) Efficient (and cachable) services from PPI could help runtime on top of the other benefits.
Reply to this email directly or view it on GitHubhttps://github.com/adamkennedy/PPI/issues/78#issuecomment-38765529 .
cross-referenced with https://rt.cpan.org/Ticket/Display.html?id=88862
Since no one seems to be inclined to do all the work and bikeshedding of getting this functionality into PPI proper, a low friction stopgap would be to release a fork of the current functions in Perl::Critic::Utils directly; I could do this quickly. It would have minimal dependencies as long as the POD functions aren't included. @petdance thoughts?
release a fork of the current functions in Perl::Critic::Utils directly;
I'm not sure what you mean by this. You'd release a distro that was just Perl::Critic::Utils?
So long as you wouldn't expect Perl::Critic to use PPIx::Stopgap, I don't see that it would affect Perl::Critic. Eventually we'd want to use the forked functions once they got a permanent home in PPI.
@Grinnz @petdance I'm low on time at the moment and can't reread all the history, but i suspect i'd prefer to have such tools separated from PPI to allow faster release schedules.
Additionally, after that it should be fairly easy to move certain things into PPI proper and slim it down while keeping its API.
I'd really like Perl::Critic to be open to using PPIx::Utils rather than forcing a stalemate until its code is in PPI.
That seems fine to me. I just don't want to convert to using the utilities PPIx::Stopgap and then again when they make their final home in PPI. If you want to make a PPIx::Utils and P::C migrate to that, that's fine with me.
Note that there's already a PPIx::Utilities that P::C uses a little bit. There are two functions in it, and P::C uses them both. Maybe they should be migrated to PPIx::Utils as well.
I've released this as PPIx::Utils. I reorganized the functions a bit, the is_* functions are in PPIx::Utils::Classification, and the others dealing with PPI documents are in PPIx::Utils::Traversal. I also included the two functions from PPIx::Utilities in ::Traversal. If anyone wants comaint to help with maintenance let me know, and PRs welcome.