stevan / p5-App-Critique

An incremental refactoring tool for Perl powered by Perl::Critic
3 stars 4 forks source link

Wishlist: Option to go directly into editor when violations found #22

Open flimzy opened 8 years ago

flimzy commented 8 years ago
Found 2 violations, would you like to review them? [Y/n]: 

I'd like to see this prompt changed perhaps to:

Found 2 violations, would you like to review them? [Y/n] or open in (e)ditor: 
stevan commented 8 years ago

I worry a bit that this will complicate the workflow, can you give me some thoughts as to your motivation behind this? What would you do when you open the editor? Just review and not change anything? Perhaps fix all violations at once? ... and if so, how would we expect to commit the changes? Etc. Etc.

I am not against this if we can make it work, I know I encountered a lot of cases while reviewing the PreventUnusedVariables critic policy where I could have fixed multiple violations in a single commit, but I felt having the small, isolated commits was better.

flimzy commented 8 years ago

The main motivation is that I tend to hit 'Y' on every prompt, or a minimum of twice per violating file. It's not usually until after I'm in the editor whether I know if I want to skip an edit.

With your concerns in mind, perhaps a better solution for my case would be a couple of command-line flags, one to auto-review violations, and one to auto-edit violations. The latter option would only be useful for graphical editors, where one can refer back to the review in the console, after the editor has opened the file. In a console editor, the auto-review feature wouldn't be very useful.

In lieu of a command-line option, an (A)lways option could be presented on both prompts, to take effect for the rest of the session.

stevan commented 8 years ago

Can you describe what the auto-review and auto-edit processes would be? I had an --auto feature at one point, but I think it was different then what you are thinking, either way, I am open to workflow improvements here, absolutely.

On Fri, Sep 2, 2016 at 10:05 PM, Jonathan Hall notifications@github.com wrote:

The main motivation is that I tend to hit 'Y' on every prompt, or a minimum of twice per violating file. It's not usually until after I'm in the editor whether I know if I want to skip an edit.

With your concerns in mind, perhaps a better solution for my case would be a couple of command-line flags, one to auto-review violations, and one to auto-edit violations. The latter option would only be useful for graphical editors, where one can refer back to the review in the console, after the editor has opened the file. In a console editor, the auto-review feature wouldn't be very useful.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stevan/p5-App-Critique/issues/22#issuecomment-244474996, or mute the thread https://github.com/notifications/unsubscribe-auth/AACybvjxI0EmN8e_FexB0jjozwX7GBJ7ks5qmIGXgaJpZM4JynzM .

stuartskelton commented 8 years ago

I think the difference is more like

Found 2 violations, would you like to review them? [Y/n] or (P)eek:

where peek, shows +/- 5 lines of the violation(s) in the command line, before running the editor, so you can quickly assess if the error is a quick win or a time sink.

I like the idea of there being a 'only-fix ', where you can give a list of violations you want to automatically open the editor to fix, there by having a focus session of fixing the same issue.

flimzy commented 8 years ago

Auto-review would skip the Found 2 violations, would you like to review them? [Y/n]: prompt, and automatically show the review.

Auto-edit would show the review, then immediately open the editor.

With these options combined, I would only have to interact with the critique prompts after editing.

This would be especially valuable along with my editor's built-in perlcritic features, which means that as soon as my editor is open, I'm already shown the perlcritic violation in-editor. But for editors that don't do this, or when this isn't sufficient context, I can still refer back to the critique console window to see the preview it provided for me.

stevan commented 8 years ago

Okay, so a few things here to respond to.

@stuartskelton I like the idea of peek very much, so the point where I think maybe it should actually just be the default. I know I have found very little use in the source method of the Perl::Critic::Violation object as it never provides enough context.

As for the only-fix idea, this actually can be accomplished already in a couple ways; using the perl-critic-theme option (assuming your violations have a common theme), using a custom perl-critic-profile file. If you do it this way, and use the no-violation flag when collecting, you never have a review that you don't care about.

@flimzy so what you describe is pretty much how the old --auto feature used to work in the process command, I since refactored it in an attempt to improve the flow such that it wasn't necessary, clearly that didn't happen :)

I can certainly make the auto-review feature work, but I think I need to actually see what you mean with regards to the auto-edit feature, in particular how it interacts with the built in editor critic feature.