perlcritic - Hall of Fame & Stats

[RichardCZ]

Ich habe mir gedacht, ich jage heute während des Mittagessens alle Module im FHEM Verzeichnis (nicht rekursiv, nur oberste Ebene) durch perlcritic für "severity 5". Wer im Detail wie abgeschnitten hat, soll hier nicht das Thema sein, weil wir hier keine Prangerkultur betreiben. Aber ich kann zumindest die Module zeigen, die (für diese Severity) makellos geblieben sind.

Stand 2020-03-27
20_GUEST.pm
20_PET.pm
20_ROOMMATE.pm
44_S7_Client.pm
59_WUup.pm
98_search.pm
HMCCUConf.pm
HMConfig.pm
msgSchema.pm
myUtilsTemplate.pm (klar  )
RTypes.pm

Bei 59_WUup.pm wusste ich das ja bereits, aber jedes andere in der Liste war eine Überraschung und hat erfreut.

Und - weil das durchaus lobenswert ist - der zweite Platz mit jeweils nur einer einzigen Meldung

44_S7_S7Client.pm
98_count.pm
98_ModbusAttr.pm
MaxCommon.pm



Und jetzt ein paar anonymisierte Statistiken:

Insgesamt gibt es 19023 Meldungen. Diese teilen sich auf wie folgt:

$VAR1 = {
          'Stricture disabled' => 76,
          '"require" statement with library name as string' => 108,
          'Integer with leading zeros:' => 52,
          '"select" used to emulate "sleep"' => 120,
          'Expression form of "eval"' => 542,
          'Two-argument "open" used' => 206,
          'Subroutine prototypes used' => 11985,
          'Don\'t modify $_ in list functions' => 50,
          '"return" statement followed by "sort"' => 10,
          'Glob written as <...>' => 2,
          'One-argument "bless" used' => 1,
          '"return" statement with explicit "undef"' => 5110,
          'Loop iterator is not lexical' => 105,
          'Package declaration must match filename' => 28,
          'Code before strictures are enabled' => 34,
          'Nested named subroutine' => 20,
          'Bareword file handle opened' => 183,
          'Variable declared in conditional statement' => 381,
          'This module is deprecated' => 6
        };

2 Module sind für über 650 Meldungen verantwortlich.

Man sieht die Prototypen und "return undef" sind die größten Brocken. Zu "return undef" schreibe ich bald was.

[Wzut]

perlcritic : die Neuigier hat gesiegt , aber zum Glück nicht auf dem Testraspi , der wäre vermutlich jetzt noch am installieren .....
Habe mir dann mal meine Module vorgenommen (sind ja nicht viele) und war eigtlich angenehm überrrascht, hatte da mehr unterschiedliche rote Zeilen erwartet.
Was da angemeckert wird ist mir entweder durch die aktuellen Themen hier auch klar (return undef , Proto) bzw. hab es dann selbst gesehen.
Allerdings scheitere ich an :

Having more than one /x regexp modifier is deprecated at /usr/local/share/perl/5.24.1/Perl/Critic/Policy/ValuesAndExpressions/RequireInterpolationOfMetachars.pm line 110.

Im ganzen Modul gibt es keine Stelle mit /x , wie ist dem auf die Spur zu kommen ? 

[RichardCZ]

Allerdings scheitere ich an :Im ganzen Modul gibt es keine Stelle mit /x , wie ist dem auf die Spur zu kommen ?

https://github.com/Perl-Critic/Perl-Critic/issues/822

Das sieht aus wie ein prä Perl-5.22 Problem einer älteren Perl__Critic Version. Im Zweifelsfall kann man das also ignorieren.
Wenn Du willst, sag mir das Modul, dann schaue ich da mit 5.30.1 drüber.

[Wzut]

Dieses Issue 822 hatte ich gestern auch via Google gefunden (und natürlich nicht kapiert)
OK, ich lege das erst einmal auf die Seite  denn das Thema /x kommt später eh wieder bei  -3 (Kommentare in RegEx)
BTW, endlich finde ich auf Knopfdruck was ich noch nie leiden konnte : unbenutzte Variablen    

[RichardCZ]

Das im Issue 822 beschriebene Problem ist, dass Perl ab 5.26 in regexen nicht nur

/x
sondern auch /xx

erlaubt (aus https://perldoc.perl.org/perlre.html):

Starting in Perl v5.26, if the modifier has a second "x" within it, it does everything that a single /x does, but additionally non-backslashed SPACE and TAB characters within bracketed character classes are also generally ignored, and hence can be added to make the classes more readable.

Code Auswählen Erweitern

    / [d-e g-i 3-7]/xx
    /[ ! @ " # $ % ^ & * () = ? <> ' ]/xx
may be easier to grasp than the squashed equivalents

Code Auswählen Erweitern

    /[d-eg-i3-7]/
    /[!@"#$%^&*()=?<>']/

Und das kannte Perl::Critic noch nicht. Ja, warten wir mit /x erst noch, Regexen sind ein größeres Thema und kommen noch.

[zap]

Das Ergebnis der Auswertung hätte ich mir schlimmer vorgestellt. Der Großteil der angemahnten "Fehler" sind Prototypen (werde ich beibehalten und daher ignorieren) und return undef (für einige FHEM Funtkionen vorgeschrieben => ignore).

An den wenigen Stellen, an denen ich return undef ohne FHEM Vorgabe verwende, weiß ich, was ich tue.

Ich bleibe also entspannt und konzentriere mich wieder auf die Weiterentwicklung meiner Module.

[RichardCZ]

Das Ergebnis der Auswertung hätte ich mir schlimmer vorgestellt. Der Großteil der angemahnten "Fehler" sind Prototypen (werde ich beibehalten und daher ignorieren) und return undef (für einige FHEM Funtkionen vorgeschrieben => ignore).

Pro Tipp: Wenn Du ganz an den Anfang Deiner Module ein

## no critic

setzt, dann sind die auch umgehend komplett frei von jedweder Kritik bis hinunter zu -1.

[eki]

Da ich erst vor kurzem mein erstes Modul "abgeliefert" habe und daher gern auch lerne, was ich falsch mache, habe ich auch mal percritic benutzt. Das ergab für level 5 eigentlich genau 3 Arten von Meldungen. Zwei davon sind der Umsetzung der Empfehlungen aus dem Wiki für Modulentwicklung geschuldet (Funktionsprototypen und "return undef").
Daher wäre mein Wunsch, dass man sich das Wiki an der Stelle mal vornimmt. Wenn dort schon Dinge vorgeschlagen werden, die zu den genannten Meldungen führen, dann ist es nicht verwunderlich wenn die meisten Module solche Ergebnisse produzieren.

@zap: Nach meinem Verständnis ist das Statement "return undef" keine Vorgabe sondern nur dass die Funktion undef zurück liefert. Soweit ich verstanden habe, klappt das bei return (ohne undef) auch wenn ein Skalar als Rückgabewert bei einer Prüfung erwartet wird und sollte insofern wäre mit return (ohne wert) den Anforderungen von FHEM genüge getan und trotzdem das Problem der Prüfung auf Listen umgangen. Ich hoffe mal ich habe jetzt keinen Quatsch verstanden oder erzählt.

[vuffiraa]

Gibt es eine Empfehlung für die Überprüfung der Module mit perlcritcs? Sprich eine .perlcriticsrc?

Aus Erfahrung weiß ich, dass mit den "unwichtigeren" Schweregraden die Diskussionen über den Sinn immer länger werden

Gruß VuffiRaa

[CoolTux]

Error warnings bei 5 und 4 ab 3 nur error
So mach ich das

[RichardCZ]

$ perlcritic -4 fhem.pl | wc -l
479

$ perlcritic -4 hobo.pl | wc -l
79


400 errors/warnings weniger, zumeist entfernen von Prototypen, return undefs und explizite returns wo welche vergessen wurden, Viele der Änderungen sind erstmal nur marginal aber nicht alle. Refactoring ist ein multi-pass Prozess: Man geht x-mal über den Code und macht - hoffentlich - äquivalente Transformationen und jeder Schritt öffnet weitere Möglichkeiten.

So ganz richtig nach Buch mache ich das auch nicht, denn eigentlich sollte man eine Testsuite vor dem Refactoring haben, die einen vor Regressionen schützt. Aber hey, das ist ein Hobbyprojekt und keine Enterprise-Software. Trotzdem arbeite ich auch darauf hin diese Testsuite zu etablieren.

Zu den Änderungen, die nicht mehr ganz so marginal sind, zähle ich das Auslagern von main-Namespace Code in eigene Namespaces, siehe
https://gl.petatech.eu/root/HomeBot/-/tree/master/lib ... dort die HomeBot.pm, HomeBot/* module. Das ist ein erster Schritt zu diesem Sci-Fi von dem ich hier gesprochen habe:
https://forum.fhem.de/index.php/topic,109522.msg1035912.html#msg1035912

Das geht erstaunlich gut, denn man lagert die Funktionen in diese Module aus, exportiert diese aber (Export::Attrs ermöglicht elegantes Exportieren als subroutine-Attribut) wieder zurück in den main Namespace, so dass sie erstmal wie gehabt allen Modulen - die ja auch in main rumfleuchen - zur Verfügung stehen. => Das funktioniert viel besser als gedacht und ist IMHO ein echt gangbarer Weg für FHEM

Der Riesen-Vorteil hierbei ist, dass man die schon mal gekapselt hat und irgendwann anfangen kann selektiv die Exports zu beschneiden.

Ich lese, wie das Experiment mit der Devel-Branch mal fehlgeschlagen ist. Nuja: HoBo ist de facto nix anderes als eine heftige experimental-devel Branch. FHEM kann aus den Erkenntnissen dort Profit ziehen. Was gut klappt, kann man adoptieren, was Probleme macht, lässt man sein.

Was mich besonders erfreut ist, dass selbst einige, die offensichtlich mit mir persönlich ein Problem zu haben scheinen, ihren Code in die m.E. richtige Richtung drücken, von daher bin ich optimistisch, dass FHEM von meinem Gewurschtel hier letzten Endes profitieren wird.

[RichardCZ]

Perl Programme ändern Perl Programme. Pervers! 

$VAR1 = {
          'Package declaration must match filename' => 28,
          'Glob written as <...>' => 2,
          'Don\'t modify $_ in list functions' => 49,
          'Nested named subroutine' => 20,
          'Expression form of "eval"' => 508,
          'Stricture disabled' => 76,
          'Subroutine prototypes used' => 11754,
          '"return" statement followed by "sort"' => 10,
          'Variable declared in conditional statement' => 377,
          'This module is deprecated' => 6,
          'Bareword file handle opened' => 172,
          'Loop iterator is not lexical' => 97,
          '"require" statement with library name as string' => 108,
          '"return" statement with explicit "undef"' => 0,
          'One-argument "bless" used' => 1,
          'Two-argument "open" used' => 196,
          '"select" used to emulate "sleep"' => 120,
          'Integer with leading zeros:' => 52,
          'Code before strictures are enabled' => 33
        };

Also schon mal 5000+ perlctiric lvl5 warnings rausgehauen.

Aber - Alter Schwede - die Diffs machen dem VCS schon zu schaffen. Zum Glück macht man das einmal und dann hoffentlich nie wieder.

vvvv wessen Browser beim Klick hierauf abstürzt - ich kann nix dafür.
https://gl.petatech.eu/root/HomeBot/-/commit/af8e450f08289b4b202b05a9f1da50b3a1813309

[Christoph Morrison]

$ perlcritic -4 fhem.pl | wc -l
...
$ perlcritic -4 hobo.pl | wc -l
79

wc ist hier für die Toilette wenn du den -C Flag für perlcritic benutzt

[RichardCZ]

wc ist hier für die Toilette wenn du den -C Flag für perlcritic benutzt

$ perlcritic -C -4 hobo.pl
hobo.pl: 49

Tatsache.