#16 Minor corrections to Perl::Critic integration

closed
nobody
None
5
2011-09-13
2011-09-13
Jan Ploski
No

(Suggestions from Karel Sprenger, patch prepared by myself)

I am looking at version 0.6.28 of perleditor in EPIC 0.6.40. File SourceCritic.java contains the code:

additionalOptions.add("%f~|~%s~|~%l~|~%c~|~%m~|~%e~|~%p" + getSystemLineSeparator());

This is incorrect and should be

additionalOptions.add("%f~|~%s~|~%l~|~%c~|~%m~|~%e~|~%p%n";

The "%n" instructs perlcritic to add the system line separator at the end of each formatted line, which is what EPIC expects. The current code simply adds a useless system line separator at the end of the additionalOptions entry.

Furthermore I have my doubts about "-verbose" (shouldn't this be "--verbose"?) and the order of the perlcritic command line options. The documentation states the options should come first and the file name last.

Finally, perlcritic complains the file is not in the proper directory for a package. I noticed perlcritic.el in Emacs doesn't report this problem, but there perlcritic is invoked with the directory of the file as the current working directory.

Discussion

  • Jan Ploski
    Jan Ploski
    2011-09-13

     
    Attachments
  • Jan Ploski
    Jan Ploski
    2011-09-13

    • summary: Minor corrections to Perl::Critic --> Minor corrections to Perl::Critic integration
     
  • Jan Ploski
    Jan Ploski
    2011-09-13

    The %n is more elegant and user-friendly (for those who want to wrap around perlcritic with a shell script or some such), but has the same effect as passing the literal line separator (it may appear "useless" in shell, but works when done from Java code).

    -verbose and --verbose both seem to work, changed to --verbose to match documentation (hopefully this won't break older versions of perlcritic). Also moved script name to be checked to the end of arguments list.

    I changed the perlcritic process to run with working directory = project root, maybe this will alleviate the reported problems (couldn't reproduce this). I suspect that the correct way of doing it would be to somehow pass the @INC to perlcritic, but haven't examined it more closely.

    Patch applied in CVS.

     
  • Jan Ploski
    Jan Ploski
    2011-09-13

    • status: open --> closed
     
  • Oliver Trosien
    Oliver Trosien
    2011-09-13

    We still have a newline problem with perlcritic, but at a different location, while parsing the perlcritic output. Sometimes perlcritic rules generate output including newlines on their own. epic fails to match these lines and will simply ignore them (well, log an error probably). Adding support for this would probably lead to a more complicated multi-line regex - or maybe there's another solution, or an easy fix for that....

     
  • Jan Ploski
    Jan Ploski
    2011-09-13

    I wasn't aware of the multiline ambiguity problem. In that case we should indeed use some custom record terminator much like the funny ~|~ already used to separate fields (and keep newlines just for readability/debugging).