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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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");
...
}
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Anonymous
-
2006-10-30
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
> 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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
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.
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.
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.
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).
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.
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.
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.
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.
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.
Why the whole interactivity? Why not make Perl critic work transparently in the background like the validator?
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.
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.
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.
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.
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.
Did you read this: http://e-p-i-c.sourceforge.net/devguide/devguide.html
Specifically, the text after "Installing the Eclipse SDK and EPIC".
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!
Jan,
Thank you again. I will follow the dev guide.
Pierre.
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.
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.
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.
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
> 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.
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.