Autor Thema: my $foo = $bar if $baz; # UNDEFINED!  (Gelesen 3516 mal)

Offline Christoph Morrison

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1772
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #15 am: 13 Februar 2021, 14:54:47 »
Bei perlcritic habe ich sehr gemischte Gefühle. Ich habe mal probehalber einige meiner Projekte durch perlcritic gejagt und Tonnen von Warnings erhalten, die allesamt harmlos/unbegründet waren. Ein Tool das 99% false-positives liefert ist keine echte Hilfe, zumindest nicht wenn man das automatisieren will... Ich muss aber zugeben, dass ich perlcritic in der default-configuration verwendet habe. Vermutlich kann man das so konfigurieren, dass es sinnvollere Ergebnisse liefert.

Genau. Man kann z.B. nur einen einzelnen Test aufrufen, in diesem Falle wäre das ProhibitConditionalDeclarations.

Ich habe übrigens mit P::C die Erfahrung gemacht, dass es genau 0 false-positives geliefert hat. Dazu habe ich mir jeden Fall dann auch im Buch angeschaut und versucht nachzuvollziehen, warum P::C meckert.
« Letzte Änderung: 13 Februar 2021, 14:56:26 von Christoph Morrison »

Offline Christoph Morrison

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1772
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #16 am: 13 Februar 2021, 16:23:00 »
Das hier ist die Liste, die mir P::C ausspuckt:

./FHEM/10_MQTT_GENERIC_BRIDGE.pm
./FHEM/98_notice.pm
./FHEM/21_SONOSPLAYER.pm
./FHEM/26_KM273.pm
./FHEM/60_allergy.pm
./FHEM/98_Modbus.pm
./FHEM/98_SVG.pm
./FHEM/98_inotify.pm
./FHEM/74_UnifiVideo.pm
./FHEM/90_SIGNALduino_un.pm
./FHEM/14_SD_UT.pm
./FHEM/52_I2C_LCD.pm
./FHEM/76_SMAPortal.pm
./FHEM/50_TelegramBot.pm
./FHEM/10_FRM.pm
./FHEM/52_I2C_PCA9532.pm
./FHEM/38_netatmo.pm
./FHEM/00_HMUARTLGW.pm
./FHEM/74_AMADDevice.pm
./FHEM/77_UWZ.pm
./FHEM/RESIDENTStk.pm
./FHEM/95_YAAHM.pm
./FHEM/14_SD_BELL.pm
./FHEM/74_Nmap.pm
./FHEM/10_EIB.pm
./FHEM/98_DOIFtools.pm
./FHEM/98_telnet.pm
./FHEM/48_BlinkCamera.pm
./FHEM/22_HOMEMODE.pm
./FHEM/20_FRM_SERVO.pm
./FHEM/21_HEOSGroup.pm
./FHEM/00_TCM.pm
./FHEM/53_GHoma.pm
./FHEM/92_FileLog.pm
./FHEM/10_KNX.pm
./FHEM/95_FLOORPLAN.pm
./FHEM/00_TUL.pm
./FHEM/88_HMCCU.pm
./FHEM/98_monitoring.pm
./FHEM/93_DbRep.pm
./FHEM/21_HEOSPlayer.pm
./FHEM/20_FRM_PWM.pm
./FHEM/21_HEOSMaster.pm
./FHEM/01_FHEMWEB.pm
./FHEM/10_EnOcean.pm
./FHEM/98_PID20.pm
./FHEM/77_SMAEM.pm
./FHEM/75_msgConfig.pm
./FHEM/98_template.pm
./FHEM/98_JsonList2.pm
./FHEM/73_GardenaSmartBridge.pm
./FHEM/88_Itach_IRDevice.pm
./FHEM/59_LuftdatenInfo.pm
./FHEM/55_DWD_OpenData.pm
./FHEM/57_Calendar.pm
./FHEM/00_SONOS.pm
./FHEM/88_xs1Bridge.pm
./FHEM/93_DbLog.pm
./FHEM/00_CUL.pm
./FHEM/Unit.pm
./FHEM/98_archetype.pm
./FHEM/98_DLNARenderer.pm
./FHEM/76_msgDialog.pm
./FHEM/39_Talk2Fhem.pm
./FHEM/38_CO20.pm
./FHEM/51_Netzer.pm
./FHEM/30_HUEBridge.pm
./FHEM/44_ROLLO.pm
./FHEM/lib/ProtoThreads.pm
./FHEM/10_ZWave.pm
./FHEM/98_structure.pm
./FHEM/98_serviced.pm
./FHEM/32_speedtest.pm
./FHEM/70_VIERA.pm
./FHEM/FritzBoxUtils.pm
./FHEM/98_todoist.pm
./FHEM/12_HMS.pm
./FHEM/74_Unifi.pm
./FHEM/32_SYSSTAT.pm
./FHEM/10_Itach_IR.pm
./FHEM/40_RFXCOM.pm
./FHEM/34_ESPEasy.pm
./FHEM/72_XiaomiDevice.pm
./FHEM/96_allowed.pm
./FHEM/71_YAMAHA_NP.pm
./FHEM/10_CUL_HM.pm
./FHEM/34_SWAP.pm
./FHEM/98_powerMap.pm
./FHEM/50_HP1000.pm
./FHEM/00_Neuron.pm
./FHEM/OWX_FRM.pm
./FHEM/93_RFHEM.pm
./FHEM/96_Snapcast.pm
./FHEM/98_HMtemplate.pm
./FHEM/59_Twilight.pm
./FHEM/98_autocreate.pm
./FHEM/52_I2C_HDC1008.pm
./FHEM/50_MOBILEALERTSGW.pm
./FHEM/38_Broadlink.pm
./FHEM/00_ZWDongle.pm
./FHEM/30_LIGHTIFY.pm
./FHEM/10_RESIDENTS.pm
./FHEM/37_Spotify.pm
./FHEM/72_FRITZBOX.pm
./FHEM/52_I2C_PCA9685.pm
./FHEM/Meta.pm
./FHEM/45_TRX.pm
./FHEM/52_I2C_DS1307.pm
./FHEM/59_Weather.pm
./contrib/betateilchen/51_BBB_BMP180.pm
./contrib/betateilchen/57_Calendar.pm
./contrib/98_dev_proxy.pm
./contrib/31_PLAYBULB.pm
./contrib/98_FileLogConvert.pm
./contrib/YAF/FHEM/YAF/libs/json/JSON/backportPP.pm
./contrib/98_Sprinkle.pm
./contrib/70_ONKYO_AVR_PULL.pm
./contrib/HMCCU/FHEM/88_HMCCU.pm
./contrib/DS_Starter/76_SMAPortal.pm
./contrib/DS_Starter/93_DbRep.pm
./contrib/DS_Starter/77_SMAEM.pm
./contrib/DS_Starter/93_DbLog.pm
./contrib/86_FS10.pm
./configDB.pm
./fhem.pl

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #17 am: 13 Februar 2021, 20:19:04 »
Soweit ich sehe, habe ich durch die Aenderungen nirgendwo ein Problem gefixt.
Echt? Dann hat es ja nicht geschadet...

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #18 am: 14 Februar 2021, 01:17:38 »
Ich habe übrigens mit P::C die Erfahrung gemacht, dass es genau 0 false-positives geliefert hat. Dazu habe ich mir jeden Fall dann auch im Buch angeschaut und versucht nachzuvollziehen, warum P::C meckert.

Ich habe vielmehr das Gefühl dass perlcritic in erster Linie den Vertrieb von PBP ankurbeln soll. Diese Annahme liegt aufgrund der Ausgaben von perlcritic sehr nahe!

Hier die Ausgabe für eines meiner Projekte, diese "Fehlermeldungen" will ich weiter unten näher beleuchten:

$ perlcritic  /m/s/rep/git/public/jw/netarchive/narun
"require" statement with library name as string at line 201, column 12.  Use a bareword instead.  (Severity: 5)
"require" statement with library name as string at line 202, column 12.  Use a bareword instead.  (Severity: 5)
"require" statement with library name as string at line 203, column 12.  Use a bareword instead.  (Severity: 5)
"return" statement with explicit "undef" at line 843, column 5.  See page 199 of PBP.  (Severity: 5)
Don't modify $_ in list functions at line 1761, column 5.  See page 114 of PBP.  (Severity: 5)
Don't modify $_ in list functions at line 2834, column 5.  See page 114 of PBP.  (Severity: 5)
"require" statement with library name as string at line 3065, column 12.  Use a bareword instead.  (Severity: 5)
"return" statement with explicit "undef" at line 3283, column 5.  See page 199 of PBP.  (Severity: 5)

50% der "Fehler" beziehen sich auf "require". Also schauen wir uns diese als erstes an:

# Read config
0 ||
    eval { require "/etc/na/$conf/na.conf" } ||
    eval { require "/etc/na/$conf.conf" }    ||
    eval { require "/etc/na.conf" }          ||
    die "Can't read config '$conf'";

Hier wird require verwendet, um Konfigurationsdateien zu lesen/parsen. Wie bitte soll ich hier ein bareword verwenden? Vorschläge, das besser zu formulieren nehme ich gerne entgegen...

    eval { require "sys/resource.ph"; $prio_process = &PRIO_PROCESS; };

Auch hier würde mich interessieren, wie das als "Bareword" formuliert werden soll... Zwar könnte man ein "foo/bar.pm" als foo::bar formulieren. Aber hier ist es eben kein .pm sondern ein .ph


Weitere 25% der "Fehler"  werden in Zeilen 5 und 6 moniert:

   my @files = @$files;
   grep {s/\n//g; s/$/\n/; } @files; # ensure every entry has exactly one NL

sowie

    my @client_cmd = @{$info->{"command"}};
    grep { s/__CLIENT__/$client/g; } @client_cmd;

Da Seite 114 von PBP bei "Blick-ins-Buch" verfügbar ist, kann ich hier nachvollziehen was perlcritic hier anmoniert. Und es ist klar, dass perlcritic den entsprechende Absatz in PBP vollkommen fehlerhaft umsetzt.

In beiden Fällen wird EXPLIZIT eine Kopie gemacht, auf der grep operiert. Im Gegensatz zur Beschreibung in PBP wird die ursprüngliche Datenstruktur nicht modifiziert.

Spätestens an dem Umstand, dass der Rückgabewert von grep nicht verwendet wird, hätte für perlcritic der Hinweis sein müssen, dass der einzige Zweck dieses Konstrukts der Seiteneffekt ist die Eingabestruktur zu modifizieren.

Insofern haben sowohl perlcritic als auch PBP unrecht: die Modifikation des Arrays ist hier beabsichtigt und der einzige Zweck, dass grep hier überhaupt verwendet wird.

Und da der Rückgabewert von grep nirgendwo zugewiesen wird, ist glasklar, dass der Seiteneffekt (eben diese Modifikation des Eingabearrays) in diesem Fall beabsichtigt ist. Andernfalls wäre dieser Code wirkungslos und könnte ersatzlos gestrichen werden.

Kommen wir zu den verbleibenden 25%

Hier wird moniert, dass "return" ein "undef" liefert. HIer tue ich mir schwer, diese Argumentation nachzuvollziehen. IMHO kann es in bestimmten Situationen durchaus sinnvoll sein, im Fehlerfall "undef" zu liefern um eine klare Unterscheidung zu gültigen Rückgabewerten zu haben. Leider sagt perlcritic nicht, was daran so schlimm sein soll. Vielmehr soll ich ein Buch kaufen, um zu erfahren was das soll. Nein danke! Leider ist Seite 199 von PBP bei "Blick-ins-Buch" nicht freigeschaltet. Vielleicht kannst Du dazu einige klärende Worte beitragen?

Unterm Strich sind 75% der Meldungen von perlcritic schlicht unsinnig. Bei den verbleibenden 25% sehe ich ebenfalls nicht, was perlcritic daran zu kritisieren hat. Nur eines sehe ich glasklar: perlcritic nötigt den Anwender, PBP zu kaufen. Ich glaube ja gerne, dass das ein gutes Buch ist. Aber was perlcritic daraus macht ist noch nicht einmal ansatzweise überzeugend....

Gib mal bitte Butter bei die Fische: Was steht in PBP auf Seite 199, damit ich wenigstens zwei der acht von perlcritic gemeldeten Fehlermeldungen ernst nehmen kann...

Insgesamt verbleiben bei mir die gemischten Gefühle bei perlcritic: only noise, no signal!

perlcritic mag sinnvoll sein für jemanden der mal in perl hineinschnuppert. Darüberhinaus kann ich aber keinen echten Nutzwert erkennen.

Wie immer: YMMV!


Offline Beta-User

  • Developer
  • Hero Member
  • ****
  • Beiträge: 15761
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #19 am: 14 Februar 2021, 07:17:05 »
perlcritic.com kennst du?
Da kannst du erfahren, warum ein "return;" statt eines "return undef;" empfohlen wird (samt Erklärungen zum Rest).Wenn man sicher ist (!), dass die critic unberechtigt ist:  “##no critics qw(XYZ)" als Pflaster hin und jeder kann sehen, dass der code an der Stelle entsprechend gecheckt worden ist...
Server: HP-T620@Debian 10, aktuelles FHEM + ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn:MySensors, Weekday-&RandomTimer, Twilight,  AttrTemplate {u.a. mqtt2, mysensors, zwave}

Offline Wzut

  • Developer
  • Hero Member
  • ****
  • Beiträge: 4533
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #20 am: 14 Februar 2021, 07:25:23 »
zum Thema return undef hatte Richard einiges geschrieben -> https://forum.fhem.de/index.php/topic,109592.msg1035806/topicseen.html#msg1035806

@jw9 , schick mir mal ne PM mit einer gültigen Email Adresse von dir dann kann man auch das Seite 199 und ff Prob lösen :)
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

Offline rudolfkoenig

  • Administrator
  • Hero Member
  • *****
  • Beiträge: 24510
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #21 am: 14 Februar 2021, 10:22:25 »
Zitat
Zitat
Soweit ich sehe, habe ich durch die Aenderungen nirgendwo ein Problem gefixt.
Echt? Dann hat es ja nicht geschadet...
Das ist eine Verdrehung meiner Aussage.
Ich habe nur gesagt, dass mein "Fix" kein FHEM-Bug behoben hat. Dass ich dadurch keinen erzeugt habe, dass hoffe ich zwar, bewiesen ist das aber noch nicht.
Jedenfalls sind die vorherigen Aenderungen in "svn blame" jetzt umstaendlicher zu finden, aber was tut man nicht alles, um von automatischen Warnprogrammen seine Ruhe zu haben.

Offline Sidey

  • Developer
  • Hero Member
  • ****
  • Beiträge: 2594
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #22 am: 14 Februar 2021, 11:22:35 »
Als ich perlcritic ins Spiel brachte, hatte ich nicht geahnt, dass wieder die Diskussion ausbricht, ob best practices eine gute oder schlechte Sache sind.

Mir ging es primär um den im Betreff dargestellten "Fehler" und dass die regex auch Stellen findet, die kein Problem sind, weil es ein Kommentar ist.
Ich bin mir sicher, dass die regex optimiert werden könnte um diese false positives nicht zu finden.
Das Rad wurde aber schon erfunden und es ist rund. So bitte ich den Hinweis auf perlcritic zu verstehen.

Da perlcritic von Menschen gemacht wurde und Menschen Fehler machen, bitte keine unfehlbarkeit erwarten.

Wenn ich das Ursprüngliche Thema aufgreife, dass Variablen außerhalb von Bedingungen definiert werden sollten, dann lässt sich das vorkommen, ohne Perlcritic konfigurieren zu müssen mit folgendem Befehl auffinden:

perlcritic app/FHEM/ app/fhem.pl app/t/ -5 | grep condition

Da perlcritic noch läuft und mein Zeilenpuffer (200 Zeilen) ohnehin nicht ausreicht um es hier zu posten, sollte das jeder selbst für sich oder seinen Code machen.

Hier ein klitzekleiner Auszug der letzten Vorkommnisse und vorweg, hier werden auch Bedingungen gefunden die nicht durch das if statement ausgelöst werden.


app/FHEM/00_TCM.pm: Variable declared in conditional statement at line 525, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/00_TCM.pm: Variable declared in conditional statement at line 1202, column 13.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/72_XiaomiDevice.pm: Variable declared in conditional statement at line 340, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/72_XiaomiDevice.pm: Variable declared in conditional statement at line 3538, column 7.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/72_XiaomiDevice.pm: Variable declared in conditional statement at line 3539, column 7.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/88_Itach_IRDevice.pm: Variable declared in conditional statement at line 87, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/88_Itach_IRDevice.pm: Variable declared in conditional statement at line 158, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 862, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 7367, column 5.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 14560, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 14561, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 14562, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 14565, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 14567, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 14570, column 9.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 15312, column 7.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 15372, column 5.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/10_EnOcean.pm: Variable declared in conditional statement at line 15402, column 5.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 1547, column 5.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 1609, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 2632, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 2714, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 2718, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 2745, column 7.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 2798, column 7.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 2974, column 5.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 3012, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/22_HOMEMODE.pm: Variable declared in conditional statement at line 3013, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/98_monitoring.pm: Variable declared in conditional statement at line 111, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/98_monitoring.pm: Variable declared in conditional statement at line 190, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/98_monitoring.pm: Variable declared in conditional statement at line 420, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/32_SYSSTAT.pm: Variable declared in conditional statement at line 71, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/88_xs1Bridge.pm: Variable declared in conditional statement at line 122, column 2.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/88_xs1Bridge.pm: Variable declared in conditional statement at line 592, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/88_xs1Bridge.pm: Variable declared in conditional statement at line 653, column 2.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/60_allergy.pm: Variable declared in conditional statement at line 140, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/34_ESPEasy.pm: Variable declared in conditional statement at line 531, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/34_ESPEasy.pm: Variable declared in conditional statement at line 532, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/34_ESPEasy.pm: Variable declared in conditional statement at line 2742, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/59_LuftdatenInfo.pm: Variable declared in conditional statement at line 91, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/59_LuftdatenInfo.pm: Variable declared in conditional statement at line 184, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/59_LuftdatenInfo.pm: Variable declared in conditional statement at line 212, column 3.  Declare variables outside of the condition.  (Severity: 5)
app/FHEM/lib/ProtoThreads.pm: Variable declared in conditional statement at line 132, column 7.  Declare variables outside of the condition.  (Severity: 5)
app/fhem.pl: Variable declared in conditional statement at line 1470, column 5.  Declare variables outside of the condition.  (Severity: 5)



Und wer sich jetzt fragt, wie er das für seinen Code im Blick halten könnte, ohne dass gleich ein SVN Hook das einchecken verhindert, der könnte sich hier eine Ideen holen.

Grüße Sidey
Signalduino, Homematic, Raspberry Pi, Mysensors, MQTT, Alexa
Zustimmung Zustimmung x 1 Liste anzeigen

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #23 am: 14 Februar 2021, 11:58:34 »
perlcritic.com kennst du?
Danke für den Hinweis!

Jetzt weiss ich auch dass perlcritic in meinem Fall 100% false-positives geliefert hat. Perlcritic vergisst zu prüfen, ob die betreffende Funktion überhaupt irgendwann in einem list-context aufgerufen wird.

Zitat
Wenn man sicher ist (!), dass die critic unberechtigt ist:  “##no critics qw(XYZ)" als Pflaster hin und jeder kann sehen, dass der code an der Stelle entsprechend gecheckt worden ist...
Soweit kommt es noch, dass ich die Lesbarkeit des Codes opfere, um ein Tool ruhigzustellen das mir bislang 100% false-positives geliefert hat...

Offline Christoph Morrison

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1772
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #24 am: 14 Februar 2021, 13:19:43 »
perlcritic app/FHEM/ app/fhem.pl app/t/ -5 | grep condition

Oder einfach:
perlcritic --single-policy=ProhibitConditionalDeclarations

Offline jw9

  • New Member
  • *
  • Beiträge: 37
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #25 am: 14 Februar 2021, 13:38:03 »
Als ich perlcritic ins Spiel brachte, hatte ich nicht geahnt, dass wieder die Diskussion ausbricht, ob best practices eine gute oder schlechte Sache sind.
Sorry, aber hier hast Du mich komplett falsch verstanden!

Ich habe zu keinem Zeitpunkt irgendwelche Einwände gegen best practices vorgebracht! Ganz im Gegentei: ich halte eine gute Lesbarkeit, Verständlichkeit und vor allem auch Klarheit von Code für ein sehr wichtiges Qualitätskriterium. Da hätte so manch ein fhem-Modul durchaus Nachholbedarf.

Ich habe lediglich darauf hingewiesen, dass percritic es nicht schafft, die best practices auf die es selbst hinweist sauber zu erkennen.

Zitat
Mir ging es primär um den im Betreff dargestellten "Fehler" und dass die regex auch Stellen findet, die kein Problem sind, weil es ein Kommentar ist.
Dass da false-positives drin sind habe ich schon in dem Beitrag geschrieben, in dem diese regex zum ersten mal auftaucht. Das habe ich auch im weiteren Verlauf mehrfach wiederholt.

Zitat
Ich bin mir sicher, dass die regex optimiert werden könnte um diese false positives nicht zu finden.
Warum das keinen Sinn macht habe ich weiter oben auch schon geschrieben. Ich wiederhole das aber gerne nochmal:
  • ist eine regex nicht das richtige Mittel um hierarchische Strukturen zu parsen. Das war ein quick-n-dirty hack weil ich bei mehreren Modulen über solche Konstrukte gestolpert war und auf die schnelle sehen wollte ob das nur Zufall war oder häufiger vorkommt.
  • Solange der Anteil der False-positives unter 20% liegt, ist es sinnvoller die Zeit in das Fixen der true-positives zu stecken als die Erkennung zu optimieren.

Einen weiteren Grund möchte ich noch nachliefern:
  • perlcritic existiert (das kannte ich noch nicht als ich diesen thread eröffnete)

Wenn Du meinst man kann die regex entsprechend verbessern, möchte ich Dich nicht davon abhalten. Wäre eine gute Übung  ;). Vergiss dabei aber nicht, operatoren wie q, qq und qr zu berücksichtigen.

Hint: die perl-syntax ist enorm komplex. Jeder Versuch, das mit einer regex auch nur halbwegs sauber zu parsen ist von vornherein zum Scheitern verurteilt. Selbst emacs scheitert gelegentlich beim Syntax-highliting. Ich kenne sonst keine Aufgabe, bei der emacs scheitert ;D. Ein Highlight ist zB "m=>$foo", das von emacs gerne mal als Anfang einer regex gewertet wird. perl-code zu 100% korrekt parsen kann IMHO nur perl höchstselbst.

Zitat
Das Rad wurde aber schon erfunden und es ist rund. So bitte ich den Hinweis auf perlcritic zu verstehen.

ACK!

Und meinen Hinweis auf perlcritic möchte ich so verstanden wissen, dass das nicht der Weisheit letzter Schluss anzusehen ist.

Ich möchte niemandem ausreden perlcritic zu verwenden. Ganz im Gegenteil: wenn man perlcritic auf den fhem-tree loslässt, dann wirft es Tonnen an Warnings. Zu meinem Erstaunen muss ich bei den meisten davon perlcritic zustimmen:

root@kiste:/opt/fhem# find -name '*.p[lm]' -print0 | xargs -0 perlcritic -5 |wc -l
24082
root@kiste:/opt/fhem#

So langsam wird mir auch klar, warum perlcritic bei mir nur false-positives zutage fördert: die allermeisten Konstrukte die es anmoniert meide ich wie der Teufel das Weihwasser  8)

Zitat
Da perlcritic von Menschen gemacht wurde und Menschen Fehler machen, bitte keine unfehlbarkeit erwarten.
Das erwarte ich auch nicht. Aber oben war von einem commit-hook die Rede. Ich würde mich herzlich bedanken, wenn mir ein tool aufgrund von 100% false-positives den commit verweigert...

Offline Beta-User

  • Developer
  • Hero Member
  • ****
  • Beiträge: 15761
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #26 am: 14 Februar 2021, 13:55:52 »
Ich möchte niemandem ausreden perlcritic zu verwenden. Ganz im Gegenteil: wenn man perlcritic auf den fhem-tree loslässt, dann wirft es Tonnen an Warnings. Zu meinem Erstaunen muss ich bei den meisten davon perlcritic zustimmen:
Ebend.

Wer der Oberchecker ist, wird ggf. viele "Fehler" von vornherein nicht machen, mir als DAM hat es geholfen, potentiell (!) problematische Stellen zu finden und (weitestgehend) zu beseitigen, meistens sogar zugunsten der Lesbarkeit (die paar ##no critics - Stellen stören auch nicht weiter...), und dabei "nebenbei" das eine oder andere zu lernen.
(Ich habe nur Code von früheren Maintainern übernommen und nichts signifikantes wirklich selbst entwickelt; die "Ehemaligen" waren allesamt Leute, die mehr draufhaben/-hatten wie meinereiner, und ich hätte nicht gedacht, dass da so viel "critics"-Potential drin steckt...).
Aber klar: es ist ein Hilfsmittel, und wer es nicht braucht oder nicht mag oder aus bestimmten Gründen an der einen oder anderen Stelle eine andere Philosophie fährt: Es ist Perl, und da gab es doch das "there's more...".
Server: HP-T620@Debian 10, aktuelles FHEM + ConfigDB | CUL_HM (VCCU) | MQTT2: MiLight@ESP-GW, BT@OpenMQTTGw | MySensors: seriell, v.a. 2.3.1@RS485 | ZWave | ZigBee@deCONZ | SIGNALduino | MapleCUN | RHASSPY
svn:MySensors, Weekday-&RandomTimer, Twilight,  AttrTemplate {u.a. mqtt2, mysensors, zwave}

Offline Christoph Morrison

  • Developer
  • Hero Member
  • ****
  • Beiträge: 1772
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #27 am: 28 Februar 2021, 13:53:10 »
Ich möchte hier darauf hinweisen, dass mit Perl 5.30 ff. dieses Konstrukt einen fatal error produzieren wird. Flankierend dazu kommt, dass mit der nächsten Stable-Version von Debian (Bullseye) Perl 5.32 als Standard ausgeliefert werden wird, das umschließt dann früher oder später auch Raspbian und andere Debian-Derivate und damit auch einen größeren Kreis der FHEM-Userbasis.

Macht bitte entsprechende Änderungen in euren Modulen / im Core / in Libs.

Offline rudolfkoenig

  • Administrator
  • Hero Member
  • *****
  • Beiträge: 24510
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #28 am: 01 März 2021, 09:08:04 »
Zitat
Ich möchte hier darauf hinweisen, dass mit Perl 5.30 ff. dieses Konstrukt einen fatal error produzieren wird.
Das kann ich nicht bestaetigen, jedenfalls gilt die Aussage nicht generell.
Ich hatte diese Konstrukte aus meinen Modulen entfernt, ich hatte aber vorher keine Warnungen, oder "fatal error".
Womoeglich lag es daran, dass in meinen Faellen dieser Konstrukt nicht der Schleife war. Ja, ich teste aktuell mit Perl 5.32.

Offline justme1968

  • Developer
  • Hero Member
  • ****
  • Beiträge: 20836
Antw:my $foo = $bar if $baz; # UNDEFINED!
« Antwort #29 am: 01 März 2021, 09:31:03 »
@jw9: danke. ich denke ich habe meine alle repariert.
FHEM5.4,DS1512+,2xCULv3,DS9490R,HMLAN,2xRasPi
CUL_HM:HM-LC-Bl1PBU-FM,HM-LC-Sw1PBU-FM,HM-SEC-MDIR,HM-SEC-RHS
HUEBridge,HUEDevice:LCT001,LLC001,LLC006,LWL001
OWDevice:DS1420,DS18B20,DS2406,DS2423
FS20:fs20as4,fs20bs,fs20di
AKCP:THS01,WS15
CUL_WS:S300TH

 

decade-submarginal