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)

History Show all quoted text
  Fri Aug 27 11:04:14 2010 o.trosien [...] epages.com - Ticket created
Subject: Extend Perl::Critic::Policy to support refactoring
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
  Fri Aug 27 19:07:32 2010 perl [...] galumph.com - Correspondence added
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>
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.
  Fri Aug 27 19:07:33 2010 The RT System itself - Status changed from 'new' to 'open'
  Sun Aug 29 22:26:32 2010 perl [...] galumph.com - Correspondence added
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.
  Sun Aug 29 22:27:07 2010 ELLIOTJS [...] cpan.org - Status changed from 'open' to 'rejected'
  Mon Aug 30 16:52:49 2010 jeff [...] imaginative-software.com - Correspondence added
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>
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. >
  Mon Aug 30 16:52:50 2010 The RT System itself - Status changed from 'rejected' to 'open'
  Thu Sep 02 02:57:39 2010 o.trosien [...] epages.com - Correspondence added
From: o.trosien [...] epages.com
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.
  Thu Sep 02 07:38:54 2010 perl [...] galumph.com - Correspondence added
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>
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.
  Fri Sep 03 19:49:23 2010 jeff [...] imaginative-software.com - Correspondence added
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>
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
  Mon Sep 13 05:18:27 2010 o.trosien [...] epages.com - Correspondence added
From: o.trosien [...] epages.com
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