Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 60801
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: o.trosien [...] epages.com
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: (no value)
Fixed in: (no value)



Subject: Extend Perl::Critic::Policy to support refactoring
Download (untitled) / with headers
text/plain 968b
Most perlcritic violations have one or more possible resolutions, which can usually be reached by rewriting the code. Or - that's the new idea - using the refactoring possibilities in the PPI-Tree. By making it possible for Policy creators to write a resolution directly where the problems were found, we could make Perl IDEs almost as powerful as Java IDEs. For example, EPIC has a good perlcritic integration. If a perlcritic violation is found, I'd like to be able to fix this problem by applying the suggested refactoring, whenever the perlcritic policy supports this. In a nutshell we need to extend the perlcritic policy interface to return a list of possible resolutions e.g. RequireTrailingCommas has one resolution ( "addCommas" ) which produces a refactored PPI tree internally, containing the additional commas, if called with the appropriate switch ("-refactor RequireTrailingCommas=addCommas"). And finally the refactored tree is dumped to STDOUT
Subject: Re: [rt.cpan.org #60801] Extend Perl::Critic::Policy to support refactoring
Date: Fri, 27 Aug 2010 18:07:16 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 482b
I vote against this. There's a serious problem with rewriting in the middle of a run: there are multiple policies running at a time. What is one Policy supposed to do with the result of a PPI::Document after another Policy has mucked with it? What's the output supposed to look like? Perl::Critic is read-only. If you want to modify a file via PPI, it's easy enough to write a program that pulls a file in, does what it wants to the tree, and spits it back out. I've done it.
Subject: Re: [rt.cpan.org #60801] Extend Perl::Critic::Policy to support refactoring
Date: Sun, 29 Aug 2010 21:26:23 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Since there have been no arguments from any of the other developers, I'm going to reject this.
Subject: Re: [rt.cpan.org #60801] Extend Perl::Critic::Policy to support refactoring
Date: Mon, 30 Aug 2010 10:52:24 -1000
To: "bug-Perl-Critic [...] rt.cpan.org" <bug-Perl-Critic [...] rt.cpan.org>
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 413b
I'd like to have a discussion about this idea. But I'm in Maui this week. I'll send my thoughts when I get back. -Jeff On Aug 29, 2010, at 4:26 PM, "Elliot Shank via RT" <bug-Perl-Critic@rt.cpan.org Show quoted text
> wrote:
Show quoted text
> Queue: Perl-Critic > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=60801 > > > Since there have been no arguments from any of the other developers, > I'm going to reject this. >
From: o.trosien [...] epages.com
Download (untitled) / with headers
text/plain 364b
It's just my 2 cents here, but in order to do refactoring on a problem reported by a perlcritic rule, you will now first have to copy&paste the complete code of the rule. Does that sound right to you? To add some more thoughts here: Maybe there could be a plugin-architecture, so that an external module can do the refactoring when perlcritic finds a violation.
Subject: Re: [rt.cpan.org #60801] Extend Perl::Critic::Policy to support refactoring
Date: Thu, 02 Sep 2010 06:38:36 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 546b
On 9/2/10 1:57 AM, Oliver Trosien via RT wrote: Show quoted text
> It's just my 2 cents here, but in order to do refactoring on a problem > reported by a perlcritic rule, you will now first have to copy&paste the > complete code of the rule. Does that sound right to you?
Even if it is true in a particular case (and it isn't in all of them e.g. ProhibitInterpolationOfLiterals), that does not mean that the modification code belongs in Perl::Critic itself. You should refactor the detecting code out into something else that P::C and the other code could use.
Subject: Re: [rt.cpan.org #60801] Extend Perl::Critic::Policy to support refactoring
Date: Fri, 3 Sep 2010 16:49:10 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 1.9k
On Sep 1, 2010, at 11:57 PM, Oliver Trosien via RT wrote: Show quoted text
> Queue: Perl-Critic > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=60801 > > > It's just my 2 cents here, but in order to do refactoring on a problem > reported by a perlcritic rule, you will now first have to copy&paste the > complete code of the rule. Does that sound right to you? > > To add some more thoughts here: Maybe there could be a > plugin-architecture, so that an external module can do the refactoring > when perlcritic finds a violation.
So here are my two cents: I'd love to see automated refactoring tools for Perl. But I agree with Elliot that this falls outside the scope of Perl::Critic proper. Just off the top of my head, I can imagine an engine that accepts a P::C::Violation and returns a list of applicable Refactorings. Each of these Refactorings would be implemented in a pluggable module that declares which types of Policies it applies to (similar to the way that a P::C::Policy declares which PPI elements it applies to). The user then chooses which Refactoring to execute (if any). Hopefully, the P::C::Violation already contains sufficient information to allow the Refactoring module to do its work (specifically, a reference to the offending PPI::Element). But if we discover common code between the detection and the correction of a Violation, then I'm sure we could factor that out accordingly. As Elliot mentioned, P::C requires the document to be read-only. So this engine could only operate on one violation at a time. After each Refactoring is executed, the document would need to be rescanned. I think this means we also need to consider the likely workflow around such a tool (i.e. with Padre in particular). If you'd like to take a stab at this, I'd be happy to give you a commit bit in the Perl-Critic repository. I have some free time right now, and I'd love to collaborate with you on this. Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf
From: o.trosien [...] epages.com
Download (untitled) / with headers
text/plain 1.4k
Hello Jeff, fine - let's collect some thoughts on how to get refactoring plugged onto Perl::Critic. I have a commitment from the main developer of EPIC to support a version that can be used via command-line. I've also written to padre-dev and asked for their input. Firstly, I agree that the PPI tree should not be modified during the validation process. So passing a list of violations at the end to some downstream process seems to me the right way. The violations contain a reference to the offending PPI::Element hence a duplicate parse of the PPI-Tree is not necessary. When I think about IDE integration there's usually the problem that I want to refactor either one place or the complete file. And the IDE (at least EPIC does...) has the requirement, that it needs to integrate via command-line. You'll probably have to specify the wanted element via the line-position. Agreed to "this engine could only operate on one violation at a time. After each Refactoring is executed, the document would need to be rescanned." Talking about the output of the refactoring: I believe we should dump the refactored content to STDOUT rather than writing it to the file, so doing a refactoring should disable regular P::C output. The IDE should either replace the editor content or display a side-by-side diff. I'm not sure if padre has some more sophisticated approach to that, let's see if they can provide more ideas. Regards, Oliver


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.