Custom Search
use criticism pragma with EPIC
zorglups wrote on Thu Oct 26 17:49:26 MEST 2006:
Hi,
First of all, I must say that EPIC is a wonderful project. I'm getting addicted
to it.
I red the "Perl Best Practice" from Damian Conway. This helps me writting
code easier to maintain.
I found a few days ago the modules Perl::Critic and criticism which nicely
reports you warnings based on the rules elected by the book. This is really
nice.
For example, I just need to add the pragma at the begining of my code:
use criticism ('brutal'); ## A bit mazo, I know...
(I patched a bit the criticism.pm to report the errors exactly like 'perl
-c' does and as soon as I get a solution to make it work with EPIC, I will
submit a change request with my modified criticism.pm to the owner of that
project).
perl -c will give me something like this:
D:\sbin\pl\broadrsh\scripts>perl -c broadrsh.pl
RCS keyword '$Revision$' not found at broadrsh.pl line 1.
RCS keyword '$Source$' not found at broadrsh.pl line 1.
RCS keyword '$Date$' not found at broadrsh.pl line 1.
No 'VERSION' variable found at broadrsh.pl line 1.
Missing 'VERSION' section in POD at broadrsh.pl line 1.
Missing 'SUBROUTINES/METHODS' section in POD at broadrsh.pl line 1.
Missing 'DIAGNOSTICS' section in POD at broadrsh.pl line 1.
Missing 'CONFIGURATION AND ENVIRONMENT' section in POD at broadrsh.pl line
1.
Missing 'DEPENDENCIES' section in POD at broadrsh.pl line 1.
Missing 'INCOMPATIBILITIES' section in POD at broadrsh.pl line 1.
Missing 'BUGS AND LIMITATIONS' section in POD at broadrsh.pl line 1.
Missing 'LICENSE AND COPYRIGHT' section in POD at broadrsh.pl line 1.
Module does not end with '1;' at broadrsh.pl line 4.
Code before warnings are enabled at broadrsh.pl line 4.
broadrsh.pl syntax OK
As far as I could find (reading the help, the forum and some source code),
EPIC does run "perl -c" and parses its output in order to check the syntax.
Strangly thought, those 'critics' do not appear in the Problem view nor
are underlined in the code.
How does it come ? Does EPIC do something else than "perl -c" ?
Many thanks in advance for your help,
Pierre.
Note: As soon as we can get something working, I will post a complete How-To
so that other can use it or even better, the option 'enable critics' could
be added to EPIC.
Note2: For those who never tried the pragma criticism: if the Perl::Critic
and perl::criticism are not installed, perl will continue silently. This
is nice because you might want this pragma to be installed only on your
test environment.
jgangemi wrote on Thu Oct 26 18:09:11 MEST 2006:
i actually started working on actual Perl::Critic integration into epic (although only on a basic level), but was "blocked" by the bug where the validator cleared all markers on an epic editor, even if it did not create them (this bug has now been fixed). i've been under time constraints as of late, but my schedule should be returning back to normal soon and i can work on getting this finished up, and solicit feedback on where improvements/additional features are needed. on a side node, i've also got the same basic functionality ready for checking pod comments as well.
pguzis wrote on Thu Oct 26 18:22:03 MEST 2006:
I did the same thing with my installation. It works pretty well and both the warning markers and Problems view seem to update properly. The only downside I have noticed is that validation takes longer because an extra process is spawned for the Perl::Critic validator. I don't have the code at my current location, but let me know if you want to compare notes later. Also, the last time I checked out EPIC from CVS, I saw a bit of existing code related to Perl::Critic. It's very possible the authors already have this functionality planned.
jgangemi wrote on Thu Oct 26 18:11:19 MEST 2006:
oh - and to answer your question as to why those errors do not appear, if a "perl -c" command returns "Syntax OK", epic just continues - which makes sense since the validation thread should really only be reporting compilation errors.
jploski wrote on Thu Oct 26 21:06:19 MEST 2006:
I think it would be all right to test the output of perl -c for Perl::Critic stuff and attach markers even if the validation result says "Syntax OK". It should be (much) faster than having another thread which runs another Perl interpreter instance just for Perl::Critic. The whole validation process is very slow on the Perl side (compared to the Java parsing part). I don't think that we strictly need an option "Enable Perl::Critic" because the source code pragma essentially *does* say that you want it (well... it might be nice to also enable it globally like warnings).
zorglups wrote on Fri Oct 27 09:51:27 MEST 2006:
pguzis, jgangemi, How did you got it to work in EPIC ? Can you detail a bit ? Can you provide me any patched jar ? I don't have (yet) knowledge in java. Did you patch the criticism.pm ? My modification of the criticism.pm is like this (I will make it better when I really get it working with EPIC). #my $format = "$file: %m at line %l, column %c. %e.\n"; my $format = "%m at $file line %l.\n"; I can beta test anything and provide feedback. Many thanks in advance, Pierre.
jgangemi wrote on Fri Oct 27 17:02:10 MEST 2006:
i've integrated Perl::Critic into epic in the same way that Perl::Tidy is. the basic functionality right now is to run critic against a source file and report back it's output as marker annotations in the editor. i'm working on re-integrating my changes w/ the HEAD since i was rather out of sync, hopefully it won't take too long.
jgangemi wrote on Fri Oct 27 18:58:03 MEST 2006:
i have committed the basic integration of Perl::Critic to the HEAD. here are the steps to use it: 1) make sure Perl::Critic is installed 2) go to the EPIC preference page and enable it, specifying the correct path to the perlcritic executable if necessary 3) right click in the editor menu and choose the Critic option under the Source menu right now, all violations are being returned as "warning" markers. obviously a future enhancement is to change type based on severity and user defined options. i'm looking for feedback and additional features, etc you would like to see, as well as bugs that you may encounter.
jgangemi wrote on Fri Oct 27 19:06:55 MEST 2006:
p.s. right now, the only way to clear the critic markers is to fix the errors and run critic again. i will be working on a way to clear them w/ a menu option to rectify this.
jgangemi wrote on Fri Oct 27 21:02:27 MEST 2006:
fixed - you can clear perl critic markers manually w/o affecting anything else. i also fixed the bug where the menu item was not enabled unless you went into the preferences.
jploski wrote on Fri Oct 27 21:05:03 MEST 2006:
Why the whole interactivity? Why not make Perl critic work transparently in the background like the validator?
jgangemi wrote on Fri Oct 27 21:21:30 MEST 2006:
i can look into supporting both methods next - at the time i started working on this initially, i had planned for it to be "interactive", but had thought about providing a hook to run when the validator did, but using the same mechanism where the code is passed to critic. now i'll just look to have the validator output include critic output. from a personal standpoint, i don't want to embed that pragma in my code.
jploski wrote on Fri Oct 27 21:41:30 MEST 2006:
Sounds good. It just makes sense to me to have it behave just like 'use strict' and 'use warnings'. Especially in projects with lots of files it would be counterproductive to have to request feedback for each file manually. However, having glossed over the Perldoc page, I see no way of having _both_ the validator and perlcritic go over the source in a single run without using the pragma.
jgangemi wrote on Fri Oct 27 22:16:38 MEST 2006:
seems reasonable - i'll work on getting the validator to include the output when run w/ perl -c future versions would allow clicking in the navigator (although perhaps someday a "perl package") view to run against multiple files/entire directory to prevent the tedious task of checking each one separately. one snag is that i currently tell perlcritic to use the following format to print the results: %f:%s:%l:%c:%m:%e is it possible to get the pragma to output like that as well when called from EPIC? i'd prefer not to deal w/ two output styles, but i will if necessary.
pguzis wrote on Fri Oct 27 22:43:13 MEST 2006:
It doesn't look like you can modify the output format in criticism. my $format = "$file: %m at line %l, column %c. %e.\n"; local $Perl::Critic::Violation::FORMAT = $format; warn @violations; @violations contains Perl::Critic::Violation objects with string overloading behavior. If you haven't already, look into adding your Perl::Critic output formats into org/epic/perleditor/editors/errorsAndWarnings.properties. You may be able to accomodate both formats this way. The alternatives are to either hack criticism or request the ability to override the default format in criticism. Thankfully, Jeff is usually very receptive to change requests in Perl::Critic.
zorglups wrote on Sun Oct 29 11:50:50 CET 2006:
Whaouw... I'm just amazed.
I've been working with Optiperl for 1 years. This is a rather great soft.
Unfortunatelly, eventhough my company paid for it, we got no support and
no update for at 18 monthes.
A bit tired of its variable non-auto-completion, a decided to give EPIC
a second chance. I just cannot beleive I discarded it a 18 monthes ago.
When I posted this question on Perl::Critic, I really thought "maybe someone
will help me to find an ugly work-around". Instead I got really exact replies
and even implementation in the HEAD.
Many thanks for that.
That was for the good. For the bad: shame on me... I'm just java ignorant
and I'm getting lost importing from the CSV, getting the thing to build.
I think I tried all suggestions found in the forum.
Is there a step by step guide on how to get the latest built of EPIC in
order to beta test it and give constructive feedback ?
Let's be clear. I do not plan (yet) to dig into the code. I just would like
to 'run' the latest 'HEAD' version and provide feedback to help you in your
devlopments.
I must say that it is a bit frustrating to not be able to have my hands
on the things discussed in this thread.
I sent a small note to Perl::Critic author to see if he could accept one
more argument for the pragma:
use criticism ('brutal', "%m at $file line %l.\n"); ## I know that brutal
is a bit sado/mazo.
Looking forward to find a way to test and provide you feedback.
Many thanks in advance,
Pierre.
jploski wrote on Sun Oct 29 12:16:30 CET 2006:
Did you read this: http://e-p-i-c.sourceforge.net/devguide/devguide.html Specifically, the text after "Installing the Eclipse SDK and EPIC".
jgangemi wrote on Sun Oct 29 18:02:49 CET 2006:
how is everyone getting along with this? if anyone is having issues, could you please report back with the following: - eclipse version - did you start with the '-clean' flag thx!
zorglups wrote on Sun Oct 29 19:51:52 CET 2006:
Jan, Thank you again. I will follow the dev guide. Pierre.
zorglups wrote on Sun Oct 29 22:23:34 CET 2006:
Jan,
I got it to work. The URL you gave me is really clear. Maybe a sticky note
with the URL would save hours for others java newbies like me.
Jae,
I wrote a few perl lines and mdified my .perlcriticrc to trow me error even
when perlcritic is run without option:
[Perl::Critic::Policy::Miscellanea::RequireRcsKeywords]
severity = 5
[TestingAndDebugging::RequireUseWarnings]
severity = 5
[Perl::Critic::Policy::ValuesAndExpressions::ProhibitInterpolationOfLiterals]
severity = 5
I uninstalled and installed again the Perl::Critic and criticism modules
in order to make sure they were clear of any mods I could have tried previously.
I enabled the critic option in my EPIC preferences and gave my perlcritic
path (C:\perl\bin\perlcritic). Yeah... I'm on windoze...
I then played the "Critic code".
Indeed, I get some errors in the Error view and a few marks in the editor
not really at the right place.
Nevertheless, the error description just display a digit instead of the
text.
I went to debugging...
The critics on my stupid perl 'script' are:
D:\sbin\epic_test\epic_test\criticism_test.pl:5:1:1:RCS keywords $Id$ not
found:See page 441 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:1:1:RCS keywords $Revision$,
$HeadURL$, $Date$ not found:See page 441 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:1:1:RCS keywords $Revision$,
$Source$, $Date$ not found:See page 441 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:3:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:4:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:6:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:8:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:8:7:Useless interpolation
of literal string:See page 51 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:10:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:12:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:12:5:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:14:1:Code before warnings
are enabled:See page 431 of PBP
D:\sbin\epic_test\epic_test\criticism_test.pl:5:14:7:Useless interpolation
of literal string:See page 51 of PBP
The call flow is like this:
return critic.parseViolations(output);
--> violations[0] = violations[i] = parseLine(lines[i]);lines[0]);
----> parseLine("D:\sbin\epic_test\epic_test\criticism_test.pl:5:1:1:RCS
keywords $Id$ not found:See page 441 of PBP");
------> String[] tmp = toParse.split("\\:");
--------> tmp[0] = "D"
--------> tmp[1] = "\sbin\epic_test\epic_test\criticism_test.pl"
--------> tmp[2] = "5"
...
As you can see, my filename contains a ':' which puzzles your split and
get it display it 'wrongly'.
This explain why I get a digit and why the line number is not correct (which
explain why the markors are messed up.
My proposal would be to use:
protected List getCommandLineOpts(List additionalOptions)
{
...
additionalOptions.add("%f|%s|%l|%c|%m|%e\n");
...
}
private final Violation parseLine(String toParse)
{
String[] tmp = toParse.split("\\|");
...
}
I'm still not confident with using the '|' character because one day this
might come in the parsed line. Maybe you could try using a \t ?
Hoping this will help.
Pierre.
zorglups wrote on Mon Oct 30 00:42:26 CET 2006:
Jae,
In the code, one can read:
private void createMarker(MarkerUtilities factory, Violation violation)
{
...
attributes.put("pbp", violation.pbp);
...
}
What should I do/enable to see this information ? This does not come in
the marker popup nor in the error view.
Here are my few ideas/wishes :
- Keep it in the configuration panel and do not use the pragma. As first
I thought that I would prefer to enable the critics by writing the pragma
at the beginning of my code instead of a global option. Nevertheless this
would have some constraints:
1) Perl::Critic code would be executed at each validation. I use to set
the validation triggered as soon as the editor is idle for 2 secs. Having
Perl::Critic launched every 2 secs would be a nightmare over big modules
so I think launching the perlcritic script instead of using the pragma is
a good compromise. I would like in this case to be able to run the perlcritic
automatically when the editor is idle for more than x secs (different timer
than for the syntax validation)
2) I run the code on servers where perlcritic is not installed and my
coworker might get a bit confused by this new pragma (that does "nothing").
3) The pragma is not so "configurable" while the perlcritic script has
a few options that could be configurable.
On the other hand, it as Jan said, it will then run over all opened files
which is not a soo good think.
- Add to the configuration panel the different perlcritic options or allow
the user to specify the wanted options through a text field (you then have
to verify that he will not want to change the format (give error if the
user specifies -quiet -man -help -Version -V -list -verbose in its options
==> Actually, this sounds easier to setup a few radio buttons for the allowed
options ;o) ) !!!
- Enable "Explain error/warning" when right-clicking on a critic markor.
This would display the violation.pbp in the "explain error" view.
I would even suggest to record more info into the violation.pbp like the
output of "%f:%l (severity: %s)\n\n%r\n\n%e\n\n%d\n\n[%p]\n". This would
help a lot since the user would even never need to go back to the book.
Here after is an example of "%f:%l (severity: %s)\n\n%r\n\n%e\n\n%d\n\n[%p]\n":
criticism_test.pl:14 (severity: 5)
print "ok";
See page 51 of PBP
Don't use double-quotes or `qq//' if your string doesn't require
interpolation. This saves the interpreter a bit of work and it lets
the
reader know that you really did intend the string to be literal.
print "foobar"; #not ok
print 'foobar'; #ok
print qq/foobar/; #not ok
print q/foobar/; #ok
print "$foobar"; #ok
print "foobar\n"; #ok
print qq/$foobar/; #ok
print qq/foobar\n/; #ok
print qq{$foobar}; #preferred
print qq{foobar\n}; #preferred
[ValuesAndExpressions::ProhibitInterpolationOfLiterals]
I saw that the "Explain error and warnings" (public void explain(ArrayList
markers)) needs the MESSAGE attribute of the marker object to be filled
in. Appart from that, I don't think this should be too difficult.
I would suggest to set the MESSAGE to this violation.pbp instead of the
violation.message
I guess the "multi line aspect" of the dump given for each critic will give
you some troubles. Well, it is always a matter of EOL and field separator
choice at:
private final Violation[] parseViolations(String toParse)
{
String[] lines = toParse.split(getLineSeparator(toParse));
}
private final Violation parseLine(String toParse)
{
String[] tmp = toParse.split("\\|");
}
I think if you setup the format like this :
additionalOptions.add("%f_-_FS_-_%s_-_FS_-_%l_-_FS_-_%c_-_FS_-_%m_-_FS_-_%f:%l
(severity: %s)\n\n%r\n\n%e\n\n%d\n\n[%p]_-_EOCRITIC_-_\n");
And change slightly the split like this (correct my quoting, I just don't
know java at all (I just found that splitting with multichar token was not
as easy as in Perl)):
private final Violation[] parseViolations(String toParse)
{
String[] lines = toParse.split("_-_EOCRITIC_-_\n");
}
private final Violation parseLine(String toParse)
{
String[] tmp = toParse.split("_-_FS_-_");
}
===========================
Last thing: I couldn't "clear perl critic markers manually".
Okay, it is enough for today...
Best regards,
Pierre.
jgangemi wrote on Mon Oct 30 17:18:12 CET 2006:
pierre - thx for the feedback and input. at the moment, there is nothing you can do to get the pbp information. for now, i will have it be included in the problem view description field, but eventually it will be moved elsewhere (probably the annotation marker). i also agree that using the critic pragma may not be the best idea - it seems that the pragma does not return as much information as using the perlcritic script directly - which means no pbp information, etc unless we are going to start submitting more patches for Perl::Critic (not to mention the fact that having the pragma run each time the code is compiled is probably no good either). right now, critic will only run against a single editor at a time, there is no way to batch validate things, so there is no need to worry that it will run against everything. i'm also in the middle of adding support to run the process as background processes, so the ui thread no longer blocks on large files, and will fix the output parsing issue as well. jan committed a change that broke the clear markers action, so that may be the reason it is not working for you. i'll be addressing that in as well in this next set of changes. all your other ideas are good ones (some like the options were already planned), and i'll start looking into those once these other issues have been addressed.
jthalhammer wrote on Mon Oct 30 19:14:27 CET 2006:
Hello everyone-
Pierre sent me a link to this thread. I hope you don't mind if I join the
conversation...
Using the criticism pragma to generate the Perl::Critic warnings at the
same time that EPIC runs the perl compiler is _very_ clever. I'd be happy
to modify criticism.pm to help make this a little easier. Off the top of
my head, I'll probably just allow you to pass any of the Perl::Critic arguments
in a hashref like this:
use criticism ( {-severity => 3, -verbose => '%m at %f line %l'} );
In the long run, the criticism pragma is not the best way to integrate with
Perl::Critic. As Jae pointed out, you'll probably want to visually distinguish
the Perl::Critic annotations from the ones created by "perl -c". You may
also want to provide additional functionality to the Perl::Critic annotations,
like hyperlinking to the Perl::Critic POD, or to Damian's book on http://safari.oreilly.com.
And some people might forget to remove the pragma before putting their
code into production.
Most of all, starting a new interpreter every time you analyze the file
could become painfully slow. I don't know how difficult it would be, but
the most performant solution is to embed a perl interpreter in memory and
just talk directly to the Perl::Critic API. That way, you only have to
compile Perl::Critic (and all its dependencies) once. And this approach
would probably make it easier to run Perl::Critic over an entire batch of
files. Presumably, the integration with Perl::Tidy might also benefit from
a compile-once strategy.
If that's not feasible, then maybe we could setup perlcritic as a daemon
or named pipe. So EPIC could start perlcritic once, and then periodically
send in a bunch of souce code over STDIN or though a socket. This would
require some tweaking to perlcritic and we would need to consider the concurency
issues, but it seems prettty reasonable to me. We also have the Perl::Critic
web service at http://www.perlcritic.com. It wouldn't be my first choice,
but we could just send the source code out to the service for analysis (this
may appeal to folks who won't/can't install Perl::Critic locally).
I don't personally use EPIC (yet) and I'm still getting familair with the
EPIC source, so I apologize if I suggested anything stupid or inane. Once
I get my feet wet, I'd be happy to help out. And of course, I welcome any
suggestions and ideas you have for Perl::Critic. Feel free to send your
thoughts to mailto:dev@perlcritic.tigris.org
Cheers.
-Jeff
jploski wrote on Mon Oct 30 19:55:19 CET 2006:
> Using the criticism pragma to generate the Perl::Critic warnings at the same time that EPIC > runs the perl compiler is _very_ clever This is what I thought at first, however the fact that the compiler runs periodically to provide the on-the-fly syntax functionality makes running Perl::Critic at the same time unreasonable performance-wise. "perl -c" on Twig.pm takes 300 ms on my machine. "perlcritic" on the same file takes 15 seconds (and takes 14.5 seconds of user CPU time). Thus, I don't think background running of Perl::Critic would be a good idea for bigger projects - the kind of projects which would presumably benefit most from it. A batch run at night and the interactive mode Jae has already implemented appear more realistic. > Most of all, starting a new interpreter every time you analyze the file could become painfully slow. The startup time of perlcritic (measured on an empty module) is about 1 second. This is indeed a big overhead, but getting rid of it would not influence my decision between 'scheduled batch run' and 'automatic execution in the background'. It would be both difficult and bad for portability to run an embedded Perl interpreter in the same address space as the JVM. I considered it briefly for the validator (perl -c), and have given up on this idea due to the lack of available examples and startup overhead vs. total execution time considerations. Reusing an out-of-process Perl interpreter for perlcritic and communicating via pipes sounds reasonable, though. We could save the 1 second startup time and make the feature snappier for small files this way.
jgangemi wrote on Mon Oct 30 20:15:52 CET 2006:
i don't know that the 1 second startup is really all that noticeable. i started running the process as a user thread that can be put into the background and i can't even click the "run critic" menu item and make my way over to the cancel button before the job finishes on a relatively simple script. the other issue i see with running it as part of perl -c that was mentioned before is that if you have the validator thread set to run at a low interval, you'd be invoking critic each time as well doesn't make much sense. i think the separate automated thread on it's own timer would provide the best of both worlds. i'm still not against trying to find some way have this be a part of the 'perl -c' output, however i'm not sure it's wise to rely on the user to specify the output that epic will need to parse the file. it would be nice though, if the Perl::Critic could be enhanced to accept the options that the perlcritic script does, including the format output, then we could develop our own custom "epicPerlCritic" script to call Perl::Critic like i did with the "epicPodChecker.pl" script (although it is currently incorrect in cvs due to an fat finger paste error on my part, the gist of the idea is the same). how come no one ever tried to re-implement the interpretor in java, like they did for python, etc. then life would be grand.
jploski wrote on Mon Oct 30 20:35:24 CET 2006:
> i don't know that the 1 second startup is really all that noticeable. I now see that Jeffrey's argument was about these accumulating startup times - if you wanted to check 20 simple files for example, it would be 20 seconds vs. 1 second then - and quite noticeable. > i think the separate automated thread on it's own timer would provide the best of both worlds. Certainly better than running each time when "perl -c" runs, for sure, but I still think it would be a no-no for a project which contains files like Twig.pm. Making the CPU run hot for 15 seconds at a (more or less) random time is not acceptable. > however i'm not sure it's wise to rely on the user to specify the output that epic will need to parse the file. It would indeed be unreasonable to expect the user to enter weird format specifiers on the pragma line. In fact, the user should not be required to enter these format specifiers anywhere. It has to "just work", thanks to your (and possily Jeffrey's) effort spent on the integration. > how come no one ever tried to re-implement the interpretor in java, like they did for python, > etc. then life would be grand. Probably related to the amount of genius which went into Perl's source code ;-)
jgangemi wrote on Mon Oct 30 21:08:38 CET 2006:
i would think that if we were able to pass multiple files to critic during a run, then you would only need to load the interpreter once, so you'd still only see that 1 second hit - although i'm not sure how easy that would be in practice right now - but it's something for future investigation. off the top of my head, if we could construct a custom critic invoker script, then we could just iterate over a list of files/directories in perl space so the interpreter is only loaded once. i do agree that running the cpu at random times is unacceptable, but this would only work on editors that are open and have had changes since the last time critic was run against it. if people want it and don't mind the random cpu spikes, i don't see a reason not to add it - perhaps as an advanced/"use at own risk" type feature, but that can be a discussion for another day. i'd definitely like to work w/ jeffery to get the integration up to snuff, with the first topic of discussion being a way to provide some of the perlcritic script functionality through the Perl::Critic module (or equivalent) so we can rely on a custom epic to better control things.
jploski wrote on Mon Oct 30 21:38:03 CET 2006:
> if we could construct a custom critic invoker script, then we could just iterate over a list > of files/directories in perl space so the interpreter is only loaded once. It would be no good as far as progress reporting on the Java side is concerned. The request granularity between the Java code and Perl must be such that both adequate progress reporting and job cancellation are possible. For this reason, handing off files one by one through a pipe to the perlcritic process, which is started only once (or, for the sake of fault tolerance, from time to time) appears a good tradeoff. > i don't see a reason not to add it - perhaps as an advanced/"use at own risk" type feature, but that > can be a discussion for another day. One reason why I discourage adding features of this sort is that their mere availability influences people towards their use, sometimes at their peril. The "sharp knife" argument has to be weighted during design against the benefits of simplicity. Good tools should convey best practices. But I agree with you that this would be a discussion for another day...
jthalhammer wrote on Mon Oct 30 23:39:02 CET 2006:
Done. So you can now pass any arguments to Perl::Critic through the criticism
pragma like this:
use criticism ( {-verbose => 3, -severity => 1} );
I also added a pre-defined verbosity (3) that matches the format of "perl
-c". All of this requires the latest version of Perl::Critic. Everything
is available in our subversion repository at http://perlcritic.tigris.org/svn/perlcritic/trunk.
The username is "guest" and the password is "" (empty). Let me know if
that doesn't work.
It does suck that you have to know which -verbose level to use to make it
compatible with EPIC. I could see changing the defaults inside criticism.pm,
so that it "just works"
as Jan pointed out. I'll think about that some more.
However, I think we all agree that spawning a new perl every time is suboptimal.
Writing a wrapper around Perl::Critic may be the best way to go. In the
next release, all of the options supported by the perlcritic command can
be controlled via the Perl::Critic API. So you could write some kind of
daemon process like this and still get all the same functionality:
use Perl::Critic;
my %pc_args = (-verbose => 3);
while( $code = wait_for_input() ){
my $critic = Perl::Critic->new( %pc_args );
print $critic->critique( \$code );
}
You could use a separate thread to periodically send documents to the daemon
in the background or do it on-demand. Beware this would get significantly
more complicated if the editor is going to send multiple files at the same
time.
BTW: What are your thoughts about how EPIC users should configure Perl::Critic?
Do you just direct them to create a .perlcriticrc file somewhere? Or do
you plan to provide a GUI for configuring P::C? Could EPIC provide some
kind of hook for automatically creating a .perlcriticrc within each Perl
project?
-Jeff
jgangemi wrote on Tue Oct 31 00:05:52 CET 2006:
i think we should stay away from the 'perl -c' integration (thx for getting that work done though) b/c it will not be optimal for critic to run each time the validator thread has been invoked (which could be as often as 2 seconds). the fact that the Perl::Critic api will soon support everything that the perlcritic script does is a big plus and will help make the integration easier. while the daemon process sounds like a good idea, i'm not sure EPIC wants to be in the business of spawning and managing external processes like that. perhaps that would have to be something left up to the user to start, but EPIC could integrate w/ the daemon if it found it running, and fall back to processing each file individually otherwise. i'd consider this to be a pretty 'advanced' feature, and a discussion topic for another day. as for the .perlcriticrc file, currently it will be up to the user to create one of those. eventually support could be added for a 'per project' file, and perhaps even some kind of basic editor to create the file itself, but that feature is still a ways off.
jthalhammer wrote on Tue Oct 31 00:29:46 CET 2006:
> i think we should stay away from the 'perl -c' integration That's fine. Pierre's suggestion was a good one anyway. Now the default format is identical to 'perl -c' so it just works the way he expected.
jgangemi wrote on Tue Oct 31 21:30:52 CET 2006:
i've checked in the following updates: - the output delimiter has been changed to ~|~, so that should eliminate any problems that were being caused by using just the ':' in win32 machines - the pbp information is now included as part of the description in the problem view, this is temporary until the annotations are able to show the information and/or there is a 'explain critic message' view. - the critic process now runs as a user background job that can be canceled and does not block the ui - clearing critic markers works - critic menu items show up in the editor context menu and top level menu i've also added a "clear all epic" markers menu option as well, this will clear all markers created by epic, including those that were created by the validation (perl -c) process.
jploski wrote on Tue Oct 31 22:04:55 CET 2006:
Nice work, Jae! One question: what do you mean by the comment in SourceCritic.critique? Were you perhaps getting broken pipes there?
jgangemi wrote on Tue Oct 31 22:20:28 CET 2006:
hrm - to be honest, i can't remember. it may have been the broken pipe issue, but i thought when you had mentioned that to me originally, i and changed things to pass the content of the editor instead and it still didn't work. i'll have to go back and investigate to see if i still get that error.
zorglups wrote on Tue Oct 31 22:45:09 CET 2006:
Great. I will check out and test it. Pierre.
zorglups wrote on Thu Nov 9 00:08:50 CET 2006:
Jae, I finally got some time to try out your changes. This is fine except that when there is no critic returned by perlcritic, the job fails with: An internal error occured durint: "Executing Perl::Critic against criticism_test.pl". 1) Verbosity Level setting Thanks to Jeffrey's latest Perl::Critic::Config improvements, I can set the priority to what I want by updating the .perlcritic file. There is no need to make this as an option in the configuration panel. I would suggest you to change your working directory to the module directory before launching perlcritic in order to use the project specific .perlcriticrc file if any. I whould then still suggest to have in the configuration panel: - one button to open the default .perlcriticrc file - one button to open the project's .perlcriticrc file (which would first copy the default .perlcriticrc if not existing when the user pushes the button) - the url to http://search.cpan.org/dist/Perl-Critic/lib/Perl/Critic/Config.pm#CONFIGURATION 2) Explain error: Having the "%f:%l (severity: %s)\n\n%r\n\n%e\n\n%d\n\n[%p]\n" when I right click + Explain error is my next dream... Many thanks for this nice work, Pierre
jgangemi wrote on Thu Nov 9 15:39:28 CET 2006:
can you check the error log and report back any errors that have appeared there? can you also post your test script so i can try running critic against it myself locally. i've got some things to take care of this weekend, but i'm hoping to get a chance to spend some more time on this over the weekend - i've had an itch to do some java development since i'm stuck in perl land at the day job.
zorglups wrote on Sun Nov 12 15:12:24 CET 2006:
Jae,
My .perlcriticrc contains this:
severity = 1
Here is my ugly perl script intended to test easily the Perl::Critic in
Eclipse:
use strict;
use warnings;
my $toto;
my $tata;
print $toto;
print "test me";
print $tata;
if (1 == 1)
{
print "ok";
}
1;
When I run perlcritic from the project home directory, here is the output:
D:\sbin\epic_test\epic_test>perlcritic criticism_test.pl
RCS keywords $Id$ not found at line 1, column 1. See page 441 of PBP.
(Severity: 2)
RCS keywords $Revision$, $HeadURL$, $Date$ not found at line 1, column 1.
See page 441 of PBP. (Severity: 2)
RCS keywords $Revision$, $Source$, $Date$ not found at line 1, column 1.
See page 441 of PBP. (Severity: 2)
No "VERSION" variable found at line 1, column 1. See page 404 of PBP.
(Severity: 2)
Code not contained in explicit package at line 1, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 2, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 3, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 4, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 5, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 6, column 1. Violates encapsulation.
(Severity: 4)
Useless interpolation of literal string at line 6, column 7. See page 51
of PBP. (Severity: 1)
Code not contained in explicit package at line 7, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 8, column 1. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 8, column 5. Violates encapsulation.
(Severity: 4)
Code not contained in explicit package at line 10, column 1. Violates encapsulation.
(Severity: 4)
Useless interpolation of literal string at line 10, column 7. See page
51 of PBP. (Severity: 1)
Code not contained in explicit package at line 12, column 1. Violates encapsulation.
(Severity: 4)
There is no Error in the Eclipse Error Log but the console window contains
the following:
critic: D:\sbin\epic_test\epic_test\criticism_test.pl~|~2~|~1~|~1~|~RCS
keywords $Id$ not found~|~See page 441 of PBP
critic:
If I remove everything from the .perlcritic file, the error does not show
up. In fact, perlcritic does not report any critic.
I tried going to debug mode as I did before but since the critic runs as
job, I couldn't find the way to put breakpoints in the separate thread (job).
Best regards,
Pierre.
jgangemi wrote on Sun Nov 12 16:58:59 CET 2006:
hrm - i just tried the same thing on my machine and everything works (although i had to upgrade my Perl::Critic to make it recognize the severity value in .perlcriticrc) - all markers are created for me. are you starting eclipse w/ the '-clean' flag when you upgrade versions? clearing out the .perlcriticrc file will cause critic to use the default severity level (5), which is why nothing is reported at all. in order to set a breakpoint on the job, set the breakpoint inside the run method of the Job class. on another note, i broke the severity levels out into groups to utilize the different marker types made available by eclipse. 1,2 = info 3,4 = warn 5 = error eventually, these will all be exposed in the preferences, but i can manually tweak them for now if you'd like to see something different.
jthalhammer wrote on Thu Nov 9 10:57:17 CET 2006:
Wow! You guys are on fire. I'm going to have to try this out.
jthalhammer wrote on Thu Nov 9 12:35:56 CET 2006:
Not so much luck here. I followed the devguide and build the HEAD of org.epic.perleditor. But when I run Perl::Critic, I don't see any warnings or doo-dads on the screen. I stepped through the debugger and verified that it was invoking perlcritic and there was output, and it seemed to be parsing it, but I didn't follow it through to where it created the markers, repainted the screen, etc. Maybe I'm just looking in the wrong place. If I come up with more diagnostic information I'll send it along. -Jeff
jgangemi wrote on Thu Nov 9 15:44:24 CET 2006:
hrm - can you guys try running the following code through critic and tell me what happens: #!/usr/bin/perl my $foo = 1; print "$foo\n"; you should get two warnings in the editor over this (both 'code before strictures') warnings. if you're not, can you try restarting eclipse w/ the '-clean' flag and also let me know what version you are using (3.2 or 3.1).
zorglups wrote on Sun Nov 12 18:25:19 CET 2006:
I tried the small code as you asked. I'm running Eclipse 3.2.1 with EPIC 0.5.14 (updated from the HEAD today). Running Perl::Critic from EPIC: - brings the popup: An internal error occured during: "Executing Perl::Critic against criticism.pl". - gives me the following in the console window of the father Eclipse: critic: D:\sbin\epic_test\epic_test\criticism_test.pl~|~5~|~3~|~1~|~Code before strictures are enabled~|~See page 429 of PBP critic: - gives me nothing in the Error log of the father Eclipse. Can you quickly guide me to debug the "job" ? Pierre.
zorglups wrote on Sun Nov 12 18:26:23 CET 2006:
Forgot to mention that I ran Eclipse with the -clean flag.
jploski wrote on Sun Nov 12 18:42:38 CET 2006:
I suppose that you are referring to the version of org.epic.perleditor, not of "EPIC". The version of "EPIC" is the one found in org.epic.feature.main. The individual plug-ins are versioned independently. Even if you are referring to org.epic.perleditor, then your version should be 0.5.15 rather than 0.5.14 if you updated today. From these observations, I conclude that you have some sort of a partial update in your workspace. I suggest that you always update by using "Team / Synchronize with Repository" from the context menu after having selected the project to be updated. This way you always can see which files have changed remotely before actually transferring them to your workspace, and you do not run the risk of accidentally updating just some files while ignoring others.
jgangemi wrote on Sun Nov 12 20:05:55 CET 2006:
i actually just checked in some new changes, so make sure you update from HEAD yet again. to debug your problem, set a breakpoint inside the "doJob" method in PerlCriticAction (if you don't see the doJob method, something failed w/ the HEAD update) - when you run critic, the debugger will stop there. what version of critic do you have installed? look at the error view of the pde eclipse instance for information as well.
jthalhammer wrote on Thu Nov 9 16:45:43 CET 2006:
Methinks it might have something to do with the line endings. The -verbose format is hard-coded to end with "\n". I haven't sorted through the logic of getLineEndings, but I think it somehow ends up splitting on the output on "\r\n". I'm running Eclipse on Windows, but my perl is Cygwin. I'm not sure what shell EPIC is using to invoke commands (dos or sh?). I'll dig deeper this weekend. -Jeff
jgangemi wrote on Thu Nov 9 17:26:09 CET 2006:
i just committed a quick fix to lookup the system line delimiter instead of using the hard coded newline value. give that a try and see if it helps.
jgangemi wrote on Sun Nov 12 22:07:16 CET 2006:
i have just committed updates to allow the following: - a .perlcriticrc file per project - customizing the marker types (info, warning, error) used for serverity levels epic (eclipse actually) must see the .perlcriticrc file in order for the "per project profile" to work, so if it doesn't show up under the project folder via the navigator view, make sure to do a refresh.
jploski wrote on Sun Nov 12 22:30:30 CET 2006:
I added a programmatic refresh in SourceCritic to avoid the need for a manual one, please update from CVS.
dwballance wrote on Fri Dec 8 18:31:19 CET 2006:
I am using Eclipse + EPIC on Windows (3.2.1 and 0.5.24), with ActivePerl and Perl::Critic 0.2 installed. When I run from the command line, I get a successful output; when I run from within Eclipse, I get (from the PDE error log): eclipse.buildId=M20060629-1905 java.version=1.5.0_09 java.vendor=Sun Microsystems Inc. BootLoader constants: OS=win32, ARCH=x86, WS=win32, NL=en_US Command-line arguments: -os win32 -ws win32 -arch x86 Error Fri Dec 08 09:28:17 PST 2006 An internal error occurred during: "Executing Perl::Critic against test.pl". java.lang.ArrayIndexOutOfBoundsException: 1 at org.epic.perleditor.editors.util.SourceCritic.parseLine(SourceCritic.java:123) at org.epic.perleditor.editors.util.SourceCritic.parseViolations(SourceCritic.java:146) at org.epic.perleditor.editors.util.SourceCritic.critique(SourceCritic.java:58) at org.epic.perleditor.actions.PerlCriticAction.doJob(PerlCriticAction.java:67) at org.epic.perleditor.actions.PerlUserJobAction$1.run(PerlUserJobAction.java:99) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:58) I'm not currently configured with a java devel environment, so I haven't tried to step through the code. Does this stack trace give any clues? Thanks!!! (PS I used the test pl code recommended earlier in this thread.) Dennis
jploski wrote on Fri Dec 8 19:08:59 CET 2006:
The stack trace only shows that the perlcritic process is not returning data in the format requested and expected by EPIC. Whether it is an issue in EPIC or in perlcritic will remain unknown unless you put the (Java) debugger to work to see what exactly is going on. (I'd place my bets on a bug in EPIC, though.)
jgangemi wrote on Fri Dec 8 19:14:49 CET 2006:
can you post the script you are running critic against? i seem to recall seeing this before and thought it was fixed - i have critic version .21 installed on my machine - at some point i recall having to upgrade to fix an issue.
dwballance wrote on Fri Dec 8 20:35:37 CET 2006:
The script I used was: #!/usr/bin/perl my $foo = 1; print "$foo\n"; At the moment the only version I see available for ActivePerl is .20, so I'll try dl'ing .21 directly. (You did mention it earlier in the thread -- something related to line-endings I think.) Thanks! Dennis
dark2phoenix wrote on Thu Feb 1 15:00:39 CET 2007:
How did you solve this problem? I have the same basic environment (Windows XP, ActiveState Perl) and I'm using the latest Perl::Critic (reporting as 1.0) and criticism. I don't have a .perlcritic and I started Rational 7 (based on Eclipse 3.2.1) using the -clean option. Running perlcritic from the command line works fine, but when I run it from within EPIC 0.5.2.9 I get the same error: java.lang.ArrayIndexOutOfBoundsException: Array index out of range: 1 at org.epic.perleditor.editors.util.SourceCritic.parseLine(Unknown Source) at org.epic.perleditor.editors.util.SourceCritic.parseViolations(Unknown Source) at org.epic.perleditor.editors.util.SourceCritic.critique(Unknown Source) at org.epic.perleditor.actions.PerlCriticAction.doJob(Unknown Source) at org.epic.perleditor.actions.PerlUserJobAction$1.run(Unknown Source) at org.eclipse.core.internal.jobs.Worker.run(Unknown Source) I'm not adept enough yet to figure out how to actually build the module from the CVS (though I did get the CVS repository setup) so I'm just using the latest development version. I think it could very well be a configuration problem as I have 2 machines with exactly the same problem (and none working properly). I'm happy to provide any debug information if someone can point me to directions on how to do that.
jgangemi wrote on Thu Feb 1 16:14:40 CET 2007:
hrm - the code prints the critic output to stdout, but i'm not sure if that ends up anywhere if the plugin isn't run in the pde environment. you don't really need to worry about building code from cvs, just check it out and run it as an eclipse application - eclipse does the rest for you.
jploski wrote on Thu Feb 1 19:09:47 CET 2007:
See http://e-p-i-c.sourceforge.net/devguide/devguide.html for instructions on how to set up a debug environment with and without a CVS checkout.
dark2phoenix wrote on Sat Feb 3 04:01:01 CET 2007:
OK,
I managed to checkout org.epic.feature.main and org.epic.perleditor. Here
is the console output in the
main eclipse (one that the second one is launched from):
-------------------------------
critic: ~|~5~|~3~|~1~|~Code before strictures are enabled~|~See page 429
of PBP
critic:
-------------------------------
Here is the output when I run perlcritic on the same file directly:
-------------------------------
Code before strictures are enabled at line 3, column 1. See page 429 of
PBP. (Severity: 5)
------------------------------
It appears that the blank line at the end of the perlcritic output (\r\n)is
the issue. The error message
comes from :
String[] tmp = toParse.split("~\\|~");
Violation violation = new Violation();
violation.file = tmp[0];
--> violation.severity = parseInt(tmp[1]);
My guess is that is because there is no tmp[1] because the split can't happen.
I add some code to
the parseViolations method in SourceCritic.java to prevent parsing that
possibility:
--------------------------------------------------------------------------------------------
private final Violation[] parseViolations(String toParse)
{
String separator = getLineSeparator(toParse);
if ((toParse == null) || "".equals(toParse.trim()) || "\r".equals(toParse.trim())
|| "\n".equals(toParse.trim()) || toParse.endsWith("OK" + separator))
{
return EMPTY_ARRAY;
}
String[] rawLines = toParse.split(separator);
String[] lines = new String[0];
String[] tmpLines = new String[0];
for (int i = 0; i < rawLines.length; i++)
{
try {
Pattern regex = Pattern.compile("\\n|\\r",
Pattern.CANON_EQ);
Matcher regexMatcher = regex.matcher(rawLines[i]);
try {
rawLines[i] = regexMatcher.replaceAll("");
}
catch (IllegalArgumentException ex)
{
// Syntax error in the replacement text (unescaped $ signs?)
}
catch (IndexOutOfBoundsException ex)
{
// Non-existent backreference used the replacement text
}
} catch (PatternSyntaxException ex) {
// Syntax error in the regular expression
}
if ("".equals(rawLines[i])){
System.out.println("Skipping Line: " + Integer.toString(i));
}
else
{
int oldSize = lines.length;
tmpLines = new String[lines.length];
System.arraycopy(lines, 0, tmpLines, 0, lines.length);
lines = new String[oldSize+1];
System.arraycopy(tmpLines, 0, lines, 0, tmpLines.length);
lines[oldSize] = rawLines[i];
}
}
Violation[] violations = new Violation[lines.length];
for (int i = 0; i < (lines.length); i++)
{
System.out.println("critic(" + Integer.toString(i) + "): " +
lines[i]);
violations[i] = parseLine(lines[i]);
}
if (violations.length == 0)
{
log(StatusFactory.createWarning(getPluginId(),
"Perl::Critic violations.length == 0, output change?"));
}
return violations;
}
----------------------------------------------
Everything now appears to work properly. I don't have the facilities to
test it
on a UNIX or Mac configuration though. I also am a neophyte when it comes
to CVS
so I would rather someone else check this in after doing a little testing
and
re-writing my ugly hack.
Thanks for those pointers, they helped me get started.
jgangemi wrote on Sat Feb 3 18:29:43 CET 2007:
i'm curious as to why you're getting that extra line break in the output.
can you replace this line: System.out.println("critic: " + lines[i]);
with: System.out.println("critic: |" + lines[i] + "| length: " + lines[i].length());
and let me know the output.
in the meantime, i've submitted a fix that just checks the length of the
array after the string has been parsed, and if it doesn't contain the 6
expected items, it gets skipped.
good work.
dark2phoenix wrote on Mon Feb 5 14:35:15 CET 2007:
Updated the source from what I got in the HEAD. Here is the output from PerlCritic (I added the control characters so you could see them: -------------------------- Code before strictures are enabled at line 7, column 1. See page 429 of PBP. (Severity: 5)[return][newline] Loop iterator is not lexical at line 108, column 5. See page 108 of PBP. (Severity: 5)[return][newline] Loop iterator is not lexical at line 116, column 7. See page 108 of PBP. (Severity: 5)[return][newline] -------------------------- Here's the debug output when run against the same file using your updated code: -------------------------- critic: ~|~5~|~7~|~1~|~Code before strictures are enabled~|~See page 429 of PBP| length: 71 critic: | length: 0 critic: ~|~5~|~108~|~5~|~Loop iterator is not lexical~|~See page 108 of PBP| length: 68 critic: | length: 0 critic: ~|~5~|~116~|~7~|~Loop iterator is not lexical~|~See page 108 of PBP| length: 68 critic: | length: 0 critic: | length: 1 ------------------------- First time I ran it it didn't work but ever since then it appears to work fine. I think your patch works (and is much more elegant than mine).
anonwb wrote on Wed Jan 24 23:52:41 CET 2007:
I have a small problem with Perl::Critic with CTRL-SHIFT-C or the left-click SOURCE: there is no output from perlcritic displayed after the "Operation in Progress..." finishes. How do I get the tags to appear? I also found that C:/cygwin/bin/perlcritic will work, but /cygwin/bin/perlcritic will be accepted as a preference but fail "silently": Warning Wed Jan 24 17:32:01 EST 2007 Perl Process stderr: Can't open perl script "/cygwin/bin/perlcritic": No such file or directory But when I set it up in Preferences it likes it as a custom location, and it does not like /bin/perlcritic. Environment: WinXP Perl Executable /cygwin/bin/perl.exe Interpreter type Cygwin Perl Critic version 0.23 in cygwin's file /bin/perlcritic EPIC Source Plug-in 0.5.29 Thanks so much to everyone doing such a great job on EPIC. It is so great.
jgangemi wrote on Thu Jan 25 00:19:32 CET 2007:
the markers will automatically appear if critic reports errors - are you using a .perlcriticrc file? if yes, make sure your severity level is set properly. this simple perl snippet: --- #use strict; my $foo = 1; --- should produce a single warning marker when critic is run w/ no .perlcriticrc file available (the default severity level in this case is 3) i believe that '/cygwin/bin/perlcritic' is only valid if you are in a cygwin shell. epic is just 'shelling out' to call critic via Runtime.exec, which would never know about that cygwin specific path. i'm not sure why the preferences think '/cygwin/bin/perlcritic' is a valid location though, unless i am incorrect and somehow the lack of the 'C:' can be interpreted correctly in a windows environment (i'm on a mac). unfortunately, i am swamped w/ work at the day job and don't have time to investigate that further at the moment.
Note: The above is an archived snapshot of a forum thread. Use the original thread at sf.net to post comments.