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.

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.