Blog

Perl::Critic-Regeln umsetzen

01.12.2020 // Renée Bäcker

Gregor hat vor längerer Zeit das Modul Test::Perl::Critic::Progressive und ich vor kurzem das Modul PPI vorgestellt. In diesem Blogpost zeige ich, wie man mit PPI Perl::Critic-Regeln umsetzen kann, die dann in den Tests verwendet werden.

Nachdem ich eine Anwendung programmiert hatte, hat Gregor den Code angeschaut. Dabei ist er über dieses Stück Code gestolpert und meinte, es sähe schräg aus:

my ($doc) = grep{
    $_->{virtual_path} eq $virt_path
}@{ $app_config->{docs} || [] };

Für mich ist das völlig normal, ich nutze das häufiger. Aber ich gebe zu, dass man das schöner schreiben könnte.

Da man in diesem Beispiel nur an dem ersten Treffer interessiert ist, könnte man statt des grep die Funktion first aus dem Modul List::Util nutzen. Damit würde es dann schon so aussehen:

my $doc = first {
    $_->{virtual_path} eq $virt_path
}@{ $app_config->{docs} || [] };

Natürlich muss man am Anfang noch das Modul laden. Der Unterschied ist nur klein, aber für Einsteiger vielleicht doch einfacher, weil man hier nicht auf den Kontext achten muss. Beim grep gibt es unterschiedliche Ergebnisse, je nachdem ob man das Ergebnis in Skalar- oder im Listenkontext nutzt – einfach mal dieses Skript bei perlbanjo.com ausführen...

Das nächste was man schöner schreiben könnte, ist die Dereferenzierung der Arrayreferenz:

@{ $app_config->{docs} || [] }

Seit der Version 5.20 gibt es das (bis 5.24 experimentelle) Feature postderef . Da sieht die Schreibweise dann etwas schöner aus:

$app_config->{docs}->@*

Auch hier muss man am Anfang des Codes noch etwas einfügen, aber der Lesefluss ist in meinen Augen deutlich besser. Zum Schluss noch ein Leerzeichen zwischen das schließende } des greps und der Dereferenzierung. Insgesamt wäre es dann

my $doc = first {
    $_->{virtual_path} eq $virt_path
} $app_config->{docs}->@*;

Nachdem wir geklärt haben, welchen Code wir beanstanden wollen, und wie er schlussendlich aussehen soll, können wir uns an die Umsetzung der Regeln machen.

In meinen Augen liefert PPI::Dumper einen sehr guten Einstiegspunkt. Einfach den unerwünschten Code in allen möglichen Fassungen in den __DATA__-Bereich des Skriptes packen und ausführen. Damit sieht man, welche PPI-Knoten es gibt und an welchen man ansetzen muss. Der Dump des Perl Document Object Model (POM) in unserem Beispiel sieht auszugsweise folgendermaßen aus:

PPI::Document
  PPI::Token::Whitespace    '\n'
  PPI::Statement::Variable
    PPI::Token::Word    'my'
    PPI::Token::Whitespace      ' '
    PPI::Token::Symbol      '@list'
    PPI::Token::Whitespace      '   '
    PPI::Token::Operator    '='
    PPI::Token::Whitespace      ' '
    PPI::Token::Word    'grep'
    PPI::Token::Whitespace      ' '
    PPI::Structure::Block   { ... }

Bei jeder Perl::Critic-Regel muss man angeben, auf welchen Knoten die Regel angewandt werden soll. In diesem Fall ist das immer ein Knoten vom Typ PPI::Token::Word.

Gehen wir einen Teil nach dem anderen durch. Zuerst wollen wir first statt grep nutzen, wenn auf der linken Seite nur eine einelementige Liste angegeben ist.

Die passenden Beispiele dazu sind:

my @list   = grep { ausdruck() } @liste;           # grep ok
my $list   = grep { ausdruck() } @liste;           # grep ok
my ($list) = grep { ausdruck() } @liste;           # grep not ok
my ($list, $second) = grep { ausdruck() } @liste;  # grep ok
if( grep{ ausdruck() }@liste ) {                   # grep ok
}
# ggf. noch weitere Beispiele

Wenn wir uns den Dump anschauen, merken wir folgendes:

  • Wenn ein PPI::Token::Word mit dem Wert grep als Elternelement ein PPI::Statement::Variable hat, dann ist das potentiell interessant
  • Vergleiche Varianten der Variablenliste:

    PPI::Token::Whitespace      '\n'
    PPI::Statement::Variable
      PPI::Token::Word      'my'
      PPI::Token::Whitespace    ' '
      PPI::Structure::List      ( ... )
        PPI::Statement::Expression
          PPI::Token::Symbol    '$list'
    
    # vs
    
    PPI::Statement::Variable
      PPI::Token::Word      'my'
      PPI::Token::Whitespace    ' '
      PPI::Structure::List      ( ... )
        PPI::Statement::Expression
          PPI::Token::Symbol    '$list'
          PPI::Token::Operator      ','
          PPI::Token::Whitespace    ' '
          PPI::Token::Symbol    '$second'
    

    Es muss also eine Liste mit genau einem PPI::Token::Symbol sein, dessen Sigil ein $ ist.

Schauen wir uns das Grundgerüst für eine Perl::Critic-Regel an:

package Perl::Critic::Policy::PerlAcademy::ProhibitGrepToGetFirstFoundElement;

# ABSTRACT: Use List::Utils 'first' instead of grep if you want to get the first found element

use 5.006001;
use strict;
use warnings;
use Readonly;

use Perl::Critic::Utils qw{ :severities };

use base 'Perl::Critic::Policy';

our $VERSION = '2.02';

#-----------------------------------------------------------------------------

Readonly::Scalar my $DESC => q{Use List::Utils 'first' instead of grep if you want to get the first found element};
Readonly::Scalar my $EXPL => [  ]; 

#-----------------------------------------------------------------------------

sub default_severity     { return $SEVERITY_MEDIUM           }
sub default_themes       { return qw<perl_academy> }
sub applies_to           {
    return qw<
        PPI::Token::Word
    >;
}

Das kann so 1:1 kopiert werden. Der Paketnamen muss angepasst werden sowie die Beschreibungen.

Bei der default_severity muss man sich überlegen, wie wichtig einem diese Regel ist. Regeln können in sogenannte Themes eingeteilt werden.

Mit applies_to legt man eine Liste von Klassennamen fest, auf deren Objekte die Regel angewandt wird. Wir haben oben festgestellt, dass wir auf PPI::Token::Word-Objekte reagieren müssen.

Ob die Regel eingehalten wird, wird dann in der Methode violates geprüft. Ist alles ok, wird undef zurückgegeben, ansonsten ein Objekt, das die Regelmissachtung beschreibt:

sub violates {
    my ( $self, $elem, $doc ) = @_;

    # ... code zur pruefung der regel ...

    return $self->violation( $DESC, $EXPL, $elem );
}

Neben dem Objekt der Regel selbst bekommt man das Element (hier ein Objekt von PPI::Token::Word) und das PPI::Document-Objekt übergeben.

Als erstes prüfen wir, ob es ein grep-Befehl ist:

    # other statements than grep aren't catched
    return if $elem->content ne 'grep';

Dann interessieren wir uns nur für das grep, wenn das Ergebnis nicht in einer Variablen gespeichert wird. Dazu müssen wir in der Hierarchie des POM eine Stufe nach oben gehen:

    my $parent = $elem->parent;

    # grep in boolean or void context isn't checked
    return if !$parent->isa('PPI::Statement::Variable');

Und bei den Variablen muss eine Liste gegeben sein

    my $list = first{ $_->isa('PPI::Structure::List') } $parent->schildren;
    return if !$list;

Die Liste darf nur ein Element haben und das muss ein Scalar sein.

    my $symbols = $list->find('PPI::Token::Symbol');

    return if !$symbols;
    return if 1 != @{ $symbols };

    return if '$' ne substr $symbols->[0], 0, 1;

Sollte das alles zutreffen, ist das ein Regelverstoß:

    return $self->violation( $DESC, $EXPL, $elem );

Diese gesamte Regel ist mittlerweile auch auf CPAN bei meinen Regeln – und zwar als Perl::Critic::Policy::Reneeb::ProhibitGrepToGetFirstFoundElement – zu finden.

Die beiden anderen Punkte zum schöneren Code umzusetzen bleibt dem geneigten Leser überlassen.


Permalink:

Neues OpenSource-Commitment: Perl::Critic und Abhängigkeiten

04.09.2020 // Renée Bäcker

Aus unseren Unternehmenswerten, die wir in einem internen Wiki aufgeschrieben haben:

Förderung von Open Source. Wir nutzen Open Source und wollen etwas an die Gemeinschaft zurückgeben. Im Speziellen wollen wir uns um Perl kümmern, da uns viel an der Sprache und der Community liegt.

Wir machen schon relativ viel. So haben Gregor und Renée schon einige CPAN-Module veröffentlicht (die zum Teil in Zukunft in unserem Account PERLSRVDE auftauchen werden), helfen bei der Organisation des Deutschen Perl-Workshops und betreiben eine Art CPAN für ((OTRS)) Community Edition Erweiterungen.

Vor kurzem haben wir beschlossen, dass wir noch mehr machen wollen. Es ist jetzt vorgesehen, dass jeder von uns pro Quartal mindestens an einen Ticket bei Perl::Critic und dessen Abhängigkeiten arbeiten wird.

Klingt nicht viel, aber es ist ein weiterer Punkt, um unsere Unternehmenswerte zu leben. Und irgendwann müssen wir auch noch Geld verdienen...

Wir werden hier über unsere Arbeiten berichten. Die ersten Sachen haben wir auch schon gemacht. Bei der Ursachensuche zu Perl-Critic/Perl-Critic#917 bin ich darauf gestoßen, dass format in PPI falsch geparst wird und habe dann einen entsprechenden Fix geschrieben, der leider noch nicht integriert wurde. Außerdem wurde der Pull-Request #922 angenommen.


Permalink:

Test::Perl::Critic::Progressive – Codier-Richtlinien Schritt für Schritt durchsetzen

25.08.2020 // Gregor Goldbach

Codier-Richtlinien haben als Ziel, die Formulierung von Code zu vereinheitlichen und ihn dadurch zu verbessern. Wenn sie bei einer bestehenden Code-Basis eingeführt werden, gibt es in der Regel zu Beginn viele Verstöße. Test::Perl::Critic::Progressive ist ein Werkzeug, um die Richtlinien für Perl-Code schrittweise durchzusetzen.

Die Einhaltung von Richtlinien mit perlcritic prüfen

Das Werkzeug perlcritic ist seit vielen Jahren das Standardwerkzeug, um Perl-Code statisch zu prüfen. Eine statische Prüfung wird im Gegensatz zur dynamischen Prüfung vor der Ausführung durchgeführt; es bedeutet also, dass der Code untersucht wird, ohne ihn auszuführen. Mit perlcritic kann so die Einhaltung von Codier-Richtlinien festgestellt werden.

Das folgende Beispiel zeigt einen Aufruf von perlcritic, der eine Datei auf Einhaltung der Richtlinie CodeLayout::RequireTidy prüft und einen Verstoß meldet:

$ perlcritic -s CodeLayout::RequireTidy lib/App/perldebs.pm
[CodeLayout::RequireTidyCode] Code is not tidy at lib/App/perldebs.pm line 1.

Durch die Einhaltung von Richtlinien kann eine Team unter anderem Folgendes erreichen:

  • Das Verständnis über den Code wird verbessert, da er einheitlich geschrieben ist. Die Pflege und Erweiterung wird dadurch vereinfacht.
  • Mitarbeiter können besser und schneller eingearbeitet werden, da die Richtlinien eine Hilfestellung zur Codierpraxis geben.
  • Die Verwendung unsicherer Sprachkonstrukte kann aufgedeckt werden. Dadurch werden Sicherheitsprobleme verhindert.

Prüfen von Richtlinien als Test

Die Ablauflogik des Kommandozeilenwerkzeugs perlcritic kann über das\ Modul Test::Perl::Critic in die Testsuite eingebunden werden. Verstöße gegen die Richtlinien können so recht einfach aufgedeckt werden:

use Test::Perl::Critic;
all_critic_ok();

Eine Organisation erstellt selten zunächst ihre Richtlinien und entwickelt dann erst die Software anhand dieser Richtlinien. In der Regel ist der umgekehrte Fall der Normalfall: Eine existierende Code-Basis ist ohne Richtlinien geschrieben worden und die Organisation erhofft sich durch Durchsetzung der Richtlinien beispielsweise die oben genannten Vorteile.

Wenn nun bei vorhandener Code-Basis Richtlinien eingeführt werden, gibt es üblicherweise eine Reihe von Verstößen, da der Code vorher beliebig formuliert werden konnte. Die Entwickler sehen sich nun mit dem Problem konfrontiert, dass sie sich einerseits an die Richtlinien halten wollen oder sogar müssen, andererseits der existierende Code nicht zusätzlich zum Tagesgeschäft kurzfristig umgeschrieben werden kann.

Wie kann dieses Problem gelöst werden?

Iterative Verbesserung

Das Modul Test::Perl::Critic::Progressive kann hier eine technische Hilfestellung bieten. Es ist hiermit möglich, bei einer beliebigen Anzahl von Verstößen diese Schritt für Schritt zu beheben. Die Code-Basis kann so iterativ in eine Form überführt werden, die den Richtlinien entspricht.

Wie arbeiten dieses Modul nun?

Test::Perl::Critic::Progressive arbeitet ähnlich wie Test::Perl::Critic. Es verwendet die Prüflogik von perlcritic und sammelt beim ersten Aufruf die Anzahl der Verstöße jeder einzelnen Richtlinie. Die Sammlung wird in der Datei .perlcritic-history als Hash abgelegt:

$VAR1 = [
          {
            'Perl::Critic::Policy::CodeLayout::RequireTidy' => 10,
            'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringyEval' => 85,
            'Perl::Critic::Policy::BuiltinFunctions::RequireGlobFunction' => 0,
            'Perl::Critic::Policy::ClassHierarchies::ProhibitOneArgBless' => 2,
            ...
          }
        ];

Bei jedem weiteren Aufruf wird diese Sammlung erneut erstellt. Beide Sammlungen werden nun in einem Test miteinander verglichen. Dieser Test schlägt fehl, wenn die Anzahl von Verstößen bei einer Richtlinie angestiegen ist:

CodeLayout::RequireTidy: Got 54 violation(s).  Expected no more than 36.

Mit Test::Perl::Critic::Progressive kann man also sicherstellen, dass die Anzahl der Verstöße gegen die festgelegten Richtlinien nicht ansteigt.

Neben dieser technischen Lösung muss es noch organisatorische Änderungen geben, damit der Code auch wirklich verbessert wird. In der Regel bedeutet dies, dass den Entwicklern Zeit für die Verbesserung gegeben werden muss.

Nur dann, wenn das Team neben den technischen Lösung des Auffindens von Problemen zusätzlich noch Verfahren umsetzt, um den Code anhand der Empfehlungen der Richtlinien umzuformulieren, können die Entwickler schrittweise die Verstöße reduzieren. Sie werden dann nach und nach die Code-Basis überarbeiten und im Sinne der Richtlinien verbessern.

Best Practice

  • Grundsätzlich ist Test::Perl::Critic::Progressive durch seine iterative Arbeitsweise gut geeignet, in agilen Entwicklungsteams eingesetzt zu werden.
  • Der Basissatz von Richtlinien, die bei der Installation von perlcritic vorhanden sind, sollte auf keinen Fall vollständig eingebunden werden, da er veraltet ist.
  • Es ist ratsam, mit wenigen Richtlinien zu beginnen. Die Entwickler werden so durch schnelle Erfolgserlebnisse motiviert und erfahren den praktischen Nutzen von Richtlinien. Außerdem sollten die Entwickler bei der Auswahl der Richtlinien beteiligt werden, um die Akzeptanz zu erhöhen.
  • Das Team sollte entscheiden, ob es zunächst die Verstöße einzelner Richtlinien auf 0 verringern möchte oder ob einzelne Dateien vollständig bereinigt werden sollen.
  • Wenn das Team problematische Codierweisen entdeckt hat, sollte es für die Aufdeckung dieser Schreibweisen auf CPAN nach ergänzenden Richtlinien suchen oder eigene Richtlinien entwickeln.
  • Nach Möglichkeit sollte Test::Perl::Critic::Progressive in Versionskontrollsystemen so eingesetzt werden, dass abgelehnter Code nicht eingecheckt werden kann, um dem Entwickler schnellstmögliche Rückmeldung zu geben.
  • Unterstützend kann ein mit Test::Perl::Critic::Progressive durchgeführter Test in einem CI-System zu Beginn der Testsuite laufen, um dem Entwickler schnell Rückmeldung zu geben.
  • Test::Perl::Critic::Progressive verwendet Perl::Critic und somit die Distribution PPI. Letztere ist bei großen Codemengen langsam und führt zu langen Feedback-Zyklen. Test::Perl::Critic::Progressive sollte in einem CI-System daher nicht auf allen Quellen angewendet werden. Ist das nicht möglich, so sollte die Verwendung parallel zur restlichen Testsuite ablaufen.

Zusammenfassung

Test::Perl::Critic::Progressive ist ein Werkzeug, um Codier-Richtlinien schrittweise durchzusetzen. In Verbindung mit einem Vorgehen zum Beheben von Verstößen gegen diese Richtlinien kann es eingesetzt werden, um eine Code-Basis im Team nach und nach zu verbessern.

Links


Permalink: