PMD-CPD Analyse nach Duplikaten, Codeverdopplungen, Copy & Pastes und so

Begonnen von RichardCZ, 11 April 2020, 09:16:05

Vorheriges Thema - Nächstes Thema

rudolfkoenig

ZitatEs gibt ja zur Zeit leider keinen Test/Dev Zweig im svn...
Das ist falsch, siehe svn list svn+ssh://svn.fhem.de/branches, und keiner kraeht danach, wenn jemand einen Neuen anlegt.
Das Problem ist: es ist eine Privatveranstaltung, keiner macht sich die Muehe es zu testen. Die Entwickler nicht (die wollen ihren eigenen Kram entwickeln), und die Benutzer nicht, die wollen ein stable branch mit den neuesten Features.

Der Hintergrund zu den Maintainer-Prinzip: es hat sich nach anfaenglichen Experimenten ohne eine Richtlinie als sinnvoll erwiesen, wenn jemand weiss, was in einem Modul "los ist". Sonst baut jemand ein Feature ein, was unter anderem Namen schon gibt, und unvertraeglich mit einem dritten ist. Wenn ein Benutzer ein Problem meldet, dann zuckt der urspruengliche Entwickler sein Schulter (er hat den Mist nicht eingebaut), und der Wohlwollende auch, der will/kann gar nicht Support leisten.
Es geht darum, einen Verantwortlichen fuer ein Modul zu haben. Wenn mehrere es schaffen, das unter sich zu organisieren, ist es mir egal, aber ich will einem Maintainer garantieren, dass er die Chance hat, die Uebersicht zu behalten.

Kein Regel ohne Ausnahme: wenn ein neuer Regel (z.Bsp. bei der Formatierung der Doku) in Developer kommuniziert, und nach Monaten nicht angewendet wird, dann wird es trotzdem durchgefuehrt. Ist aber selten, und war bisher nur auf die Doku bezogen.

RichardCZ

Wie wäre es mit einem OPT-OUT?

Wer keinen Janitor in seinem Code haben möchte, macht am Anfang eine Zeile

# FHEM-TAGS: NOJANITOR

(dann können wir uns auch noch andere Tags überlegen), und dann wird der Code in Ruhe gelassen.
Ist zwar IMHO kontraproduktiv, weil es u.U. globale Aufräumarbeiten wie min/max torpedieren kann und deswegen auch OPT-OUT und nicht OPT-IN. Wer sich dem explizit entgegenstellen will, soll das auch sagen.

Aber ich verstehe die Bedenken von Maintainern schon. Vermutlich ist die ganze Geschichte - wie so vieles - einfach nur Vertrauenssache.

"Ich habe ein Modul für das ich verantwortlich bin, da will ich nicht, dass mir das einer zerschiesst."

Dann gebe ich hiermit zu Protokoll, dass diese Sichtweise eines Maintainers richtig ist. Ich würde das genauso sehen. Wenn ich für etwas verantwortlich bin und eine drite Seite grätscht rein und verursacht eine Regression, dann drücke ich vielleicht einmal das Auge zu, beim zweiten Mal ist der OPT-OUT so schnell drin, so schnell kann keiner svn commit sagen. Bzw. ich klage bitterlich und verlange nach einem besseren Janitor.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

herrmannj

Ich erlaube mir das Thema 'ein Modul - ein file' vs 'eigene libs' nochmals noch oben zu bringen. Bisher hat sich niemand gemeldet was idr entweder bedeutet 'kein Interesse' oder die Frage ist untergegangen.

Im ersteren Fall ist es ok. Von meiner Seite aus würde ich es begrüßen die Möglichkeit zu haben. In der Praxis findet es ja auch schon teilweise statt, ist aber imho nirgendwo sauber geregelt.

CoolTux

Ich wäre dafür zu versuchen einen modernen Programmierstil ein zu führen.
Klassen Unterteilung und Bibliotheken wo wir mehrfach vorkommenden Code auslagern.
Du musst nicht wissen wie es geht! Du musst nur wissen wo es steht, wie es geht.
Support me to buy new test hardware for development: https://www.paypal.com/paypalme/MOldenburg
My FHEM Git: https://git.cooltux.net/FHEM/
Das TuxNet Wiki:
https://www.cooltux.net

RichardCZ

Zitat von: herrmannj am 12 April 2020, 12:57:25
Ich erlaube mir das Thema 'ein Modul - ein file' vs 'eigene libs' nochmals noch oben zu bringen.

Ich wollte da ursprünglich bitten etwas Nachforschung zu betreiben. Sprich: gibt es diese Regel wirklich irgendwo oder ist das nur so ein Mythos?
Du konntest die ja laut eigener Aussage auch nirgends finden.

Denn für gewöhnlich ist die Verwendung von mehr als einem package pro File zwar möglich, aber wird nicht als gute Praxis angesehen.
(siehe z.B. https://perlmaven.com/packages-modules-and-namespace-in-perl)

Und vom technischen her gibt es auch nicht viel zu diskutieren. Die ganze use/require Infrastruktur von Perl geht davon aus, dass man ein

use FHEM::Washlet::TOTO;

auch in einem @INC Verzeichnis unter FHEM/Washlet/TOTO.pm vorfindet.

Ich weiß, dass ich mir mit solchen Aussagen nicht viele Freunde mache, aber gegenwärtig weiß ich echt nicht wie man in einem Perl Projekt dieses filename <-> package name schlimmer machen könnte, als es gegenwärtig in FHEM praktiziert wird. Also selbst wenn ich bewusst wollte - weiß ich nicht wie. Und meine Fantasie ist nicht gering.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

Wzut

Ich hätte auc noch was. Als ich das 38_BEOK Modul geschrieben habe brauchte ich Perl Code für das Broadlink Protokoll.
Was lag also näher als alles was ich brauchte mir aus 38_Broadlink zu "borgen" und im Kopf meines Moduls eine Zeile zu hinterlassen :
# Broadlink protocol parts are stolen from 38_Broadlink.pm :) , THX to daniel2311
Wie bitte löst man sowas elegant ? Die betroffenen Subs in eine eigene Datei auslagern ? und wer ist dann für diese Datei vernatwortlich ?
bzw. der andere Maintainer muß dabei auch mitspielen und sein Modul auch anpassen.
Maintainer der Module: MAX, MPD, UbiquitiMP, UbiquitiOut, SIP, BEOK, readingsWatcher

RichardCZ

Zitat von: Wzut am 12 April 2020, 13:44:13
Ich hätte auc noch was. Als ich das 38_BEOK Modul geschrieben habe brauchte ich Perl Code für das Broadlink Protokoll.
Was lag also näher als alles was ich brauchte mir aus 38_Broadlink zu "borgen" und im Kopf meines Moduls eine Zeile zu hinterlassen :
# Broadlink protocol parts are stolen from 38_Broadlink.pm :) , THX to daniel2311
Wie bitte löst man sowas elegant ? Die betroffenen Subs in eine eigene Datei auslagern ? und wer ist dann für diese Datei vernatwortlich ?
bzw. der andere Maintainer muß dabei auch mitspielen und sein Modul auch anpassen.

Elegant löst man sowas so, dass es ein

FHEM::Broadlink

unter FHEM/Broadlink.pm gibt

und Du machst dann in Deinem Modul ein

use FHEM::Broadlink qw(magic_sub);

Falls ich das recht verstanden habe, diskutieren wir gerade ob wir künftig elegant wollen.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

ZitatSprich: gibt es diese Regel wirklich irgendwo oder ist das nur so ein Mythos?

Wenn man ein Schreibzugriff aus SVN bekommt, kriegt man die:
ZitatStandard Belehrung:
========
- nur die eigenen Module modifizieren, sonst dem Maintainer Patches schicken.
- nach FHEM kommen nur die Module, die dokumentiert und im Forum betreut sind
  (siehe MAINTAINER.txt), sonst nach contrib.
- falls das Modul nach FHEM kommt, dann bitte das betroffene Forum (wg.
  Support) und Developer (wg. allgemeine API-Aenderungen) abonnieren.
- vor dem Einchecken alles (auch Doku, mit contrib/commandref_join.pl &
  Browser) testen, und die letzten Aenderungen mit "svn diff FHEM/MyModul.pm"
  pruefen.
- neue Verzeichnisse duerfen nur nach Ruecksprache mit mir angelegt werden, das
  Gleiche gilt fuer das Einchecken von fremden Dateien wie Bibliotheken, usw.
========

Der letzte Punkt is reingekommen, weil ich CPAN nicht doppeln, oder Bibliotheken auf Lizenz pruefen will. Weiterhin ist fhemupdate.pl absichtlich starr, sprich man kann zwar ein Verzeichnis in SVN anlegen, aber das kriegen die Benutzer noch lange nicht per update.

Ich bin offen diesen Regel aufzuweichen, wenn dabei mein Bedenken beruecksichtigt wird.

herrmannj

absolut! Eventuell kommen ja auch noch Beiträge und Gedanken weiterer devs dazu.

Verstehe ich richtig:

- "fremd" bezieht sich auf Code "von anderen", hauptsächlich wegen möglicher Risiken mit Bezug auf die Lizenz. Aber auch um kein CPAN 2 aufzumachen.
- "eigene" Libs sind von den bestehen Regeln gedeckt wenn die im /FHEM Verzeichnis landen. (in /FHEM aber eben flat, keine sub dirs)

Über ein "nicht flat" (da geht's ja in die Richtung namespaces) könnte man reden, die Rules müssten festgelegt werden.

So richtig wiedergegeben ?



RichardCZ

Zitat von: rudolfkoenig am 12 April 2020, 14:37:15
Wenn man ein Schreibzugriff aus SVN bekommt, kriegt man die:

Ok. Nichts darin verbietet, dass man zur Implementierung von Funktionalität X nicht mehr als 1 Modul verwenden darf.
Ich denke, damit wäre dieses "self-contained" erstmal abgehakt. Problematisch ist nur das kein Verzeichnis anlegen...

Ich habe da jetzt knapp zwei Stunden darüber sinniert und muss kleinlaut zugeben, dass mir eine richtig gute FHEM-kompatible Lösung nicht einfällt. Oder ich formuliere es mal um:

Wenn man sich HoBo anschaut, dann sieht man schon was ich mir als Ideale Lösung vorstelle:

Im root -Verzeichnis sind zwei Verzeichnisse lib und t dazugekommen. Ersteres soll Perl Module nach der regulären Nomenklatur (X::Y::Z = X/Y/Z.pm) enthalten, letzteres die zugehörigen Testfiles. Diese Vorgehensweise würde ich auch bei FHEM sehr stark ans Herz legen.

In lib tatsächlich nur Perl Module und nur nach o.g. Nomenklatur. Kein JavaScript, XML, .GZ und sonstiges Gedöns. (sowas gehört nach shared, data, resources oder so)

FHEM/lib werde ich entsprechend knicken, sobald ich die Gelegenheit dazu bekomme.

Mein Ziel ist, dass es mit der Zeit im FHEM Unterverzeichnis nur noch xx_Modulname.pm  main::-Interfaces (Hülsen sozusagen) gibt, die eigentlich nur ein use FHEM::MeinModul::Funktion qw(MeinModul_Init MeinModul_Define ...) machen, die Hauptlogik lebt aber unter lib/* und wird getestet von t/*




Tja ... aber das kann man alles machen, wenn man ne Installationsbasis von < 10 Diehards hat.  8)

Ich habe es mir auch noch einfacher gemacht und contrib erstmal gekillt. Folglich keine Ahnung wie man contrib vs. non-contrib handhaben könnte. Vermutlich müsste man ./lib & ./t nur für Modulautoren die ein FHEM/xx_Frontend.pm haben freigeben und für contrib Code sperren, womit man aber ggf. Contrib torpediert. Ist ein wenig Zweiklassengesellschaft, wofür ich keine guten Konzepte habe. Ich kann das nicht einmal mit staging/production abbilden.

Module unter ./lib anlegen würde ich nicht von "nach Rücksprache mit Nadelöhr" abhängig machen, sondern von "gemäß Nomenklatur/Hierarchie und announcement in <FHEM Forum/Development/Namespace>", aber der Modulautor macht einfach. Ähnlich wie PAUSE bei CPAN.




Auf jeden Fall, sollte das Ziel sein, dass ein Modulautor, der heute - Beispiel aus der Rippe geschnitten -

70_ONKYO_AVR.pm
71_ONKYO_AVR_ZONE.pm
ONKYOdb.pm


maintained, künftig folgendes nicht nur machen darf, sondern als "best practice" präsentiert bekommt:

* es gibt ein 70_ONKYO_AVR.pm modul, das ist aber nur ein <10-20 Zeilen Skelett

package main;

use Multimedia::AVR::ONKYO  qw(Init Define ...);

<ggf notify defs>

1;


* unter lib gibt es dann

lib/Multimedia/AVR/ONKYO.pm
lib/Multimedia/AVR/ONKYO/Zone.pm
lib/Multimedia/AVR/ONKYO/Db.pm


welche die eigentliche Codelogik in gekapselten Namespaces bereitstellen.  Ob man dann 71_ONKYO_AVR_ZONE.pm knickt oder nicht sei dahingestellt.




Gemäß FHEM Kulturgepflogenheiten ist das alles aber kein Muss. Wenn ein Autor/Maintainer alles im xx_Modulname.pm im main:: Namespace belassen will, belässt er es einfach dabei. Aber wer sein Modul auf ein höheres Level hieven will, kann das dann ja machen.

Beide Arten von Modulen können nebeneinander koexistieren. Mit der Zeit kommt vermutlich Darwin zu Wort.




Sprung in die ferne Zukunft, der Mars ist besiedelt, FHEM bei Version 97.1

Die Enkel des ONKYO Maintainers haben das nun auch gemacht, ebenso haben das die Enkel der YAMAHA_AVR und PONEER_AVR Modulmaintainer gemacht.
Nun kommen die Enkel von RichardCZ und behaupten man könne alles viel besser machen und knicken den verbleibenden Rest von xx_Frontend Modulen, bauen stattdessen ein dynamisches Laden von Modulen mit rekursivem Absiteg in die ./lib/* Hierarchie und machen z.B. ein abstraktes Modul

Multimedia/AVR.pm

was der werte User von FHEM 97.1 als AVR Device definiert und es ist ihm komplett wurst welches Gerät von welchem AVR Hersteller er hat.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

rudolfkoenig

ZitatSo richtig wiedergegeben ?
Ja.

Ich haette gerne eine Loesung, was die Regeln per Code sicherstellt, ich will nicht taeglich schauen, dass jemand alles was fuer DateTime::Event::Sunrise benoetigt wird, eingecheckt hat und ausliefert.
Einchecken alleine ist nicht so schlimm: wenn es nicht ausgeliefert wurde, kann man es leicht entfernen.
Update kann keine Dateien (mehr) loeschen, nur verschieben, und selbst dabei habe ich ein schlechtes Bauchgefuehl.

RichardCZ

Zitat von: rudolfkoenig am 12 April 2020, 17:08:18
Ja.

Ich haette gerne eine Loesung, was die Regeln per Code sicherstellt, ich will nicht taeglich schauen, dass jemand alles was fuer DateTime::Event::Sunrise benoetigt wird, eingecheckt hat und ausliefert.

Ich hätte im Angebot:

* geradezu faschistische SVN pre-commit, post-commit und pre-vomit  ;) hooks. Aber ich bezweifle, dass die FHEM kulturkompatibel sind
* Die Möglichkeit für CI/CD Pipelines - https://gl.petatech.eu/root/HomeBot/pipelines (selber für HoBo noch nicht eingerichtet, aber das Werkzeug ist da)
https://docs.gitlab.com/ee/ci/introduction/

Sollte FHEM e.V. doch noch über eine self-hosted GitLab Infrastruktur nachdenken: https://www.turnkeylinux.org/gitlab
Läuft bei mir unter Proxmox, war in 30 Minuten eingerichtet. Updates seit Version 11 problemstlos.

Jaja - ich weiß - gestern erst noch von CVS auf SVN migriert und jetzt wieder so neumodisches Zeug.  ;)

Um auf das DateTime::Event::Sunrise zurückzukommen:

da gibt es natürlich ein

t/DateTime/Event/Sunrise/00_smoke.t

Das kann ein einfaches use auf das Modul machen.
und wenn das in so einer CI pipeline auf die Schnauze fliegt, kann man ja dem Modulautor um 2 Uhr früh via SMS Bescheid geben.


Zitat
Update kann keine Dateien (mehr) loeschen, nur verschieben, und selbst dabei habe ich ein schlechtes Bauchgefuehl.

Tja. Rollback is king. Statt:

my $destdir="/var/www/html/fhem.de";

eben

my $destdir="/var/www/html/fhem.de/active";

und unter /var/www/html/fhem.de sieht es eben aus wie

active -> 6.0
6.0
6.1
6.2

wenn einer putt ist, schaltet man zurück.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

DS_Starter

Ich brauche mal kurz einen Anstoß.
Wie kann man das PMD-CPD Analysetool aufrufen ?

Ein paar Stellen in meinen Modulen die ich diesbezüglich schon einige Zeit verbessern will kenne ich auch ohne das Ding,
aber nicht alle.

Grüße,
Heiko
ESXi@NUC+Debian+MariaDB, PV: SMA, Victron MPII+Pylontech+CerboGX
Maintainer: SSCam, SSChatBot, SSCal, SSFile, DbLog/DbRep, Log2Syslog, SolarForecast,Watches, Dashboard, PylonLowVoltage
Kaffeekasse: https://www.paypal.me/HMaaz
Contrib: https://svn.fhem.de/trac/browser/trunk/fhem/contrib/DS_Starter

RichardCZ

Zitat von: DS_Starter am 16 Mai 2020, 18:17:42
Ich brauche mal kurz einen Anstoß.
Wie kann man das PMD-CPD Analysetool aufrufen ?

Ein paar Stellen in meinen Modulen die ich diesbezüglich schon einige Zeit verbessern will kenne ich auch ohne das Ding,
aber nicht alle.

z.B.

/usr/bin/pmd-cpd --minimum-tokens 100 --files FHEM/ --language perl


im FHEM root-Verzeichnis. Dann geht er rekursiv alle "Module" in FHEM durch.
Witty House Infrastructure Processor (WHIP) is a modern and
comprehensive full-stack smart home framework for the 21st century.

DS_Starter

ESXi@NUC+Debian+MariaDB, PV: SMA, Victron MPII+Pylontech+CerboGX
Maintainer: SSCam, SSChatBot, SSCal, SSFile, DbLog/DbRep, Log2Syslog, SolarForecast,Watches, Dashboard, PylonLowVoltage
Kaffeekasse: https://www.paypal.me/HMaaz
Contrib: https://svn.fhem.de/trac/browser/trunk/fhem/contrib/DS_Starter