98_RandomTimer.pm Code Review und packages

[Beta-User]

Kein Ding.

Ich wollte an der Stelle nur nicht einerseits CoolTux "die Arbeit getan haben lassen", und dann nichts zu der erarbeiteten Lösung zurückmelden.

Also: Nehmt euch die Zeit, die erforderlich ist, Stückwerk bringt uns nur bedingt weiter.

(auch wenn ich annehme, dass man den Import-Teil ggf. nur anders schreiben muß, also auch das nur eine kleine Aktion sein wird...)

[Beta-User]

So,

anbei mal eine Version, die durch die percritic durch ist und immer noch zu funktionieren scheint .

ABER: Es gibt zwei Dinge, die nicht sooo einfach sind, und eine der beiden  hat mit packages zu tun, daher stelle ich mal alle diesbezüglichen Fragen hier ein. Datei ist im Anhang, das Validierungssystem läuft mit Perl 5.26.

Angemakelt wird:

Code Auswählen Erweitern

perlcritic -3 98_RandomTimer.pm
Multiple "package" declarations at line 48, column 1.  Limit to one per file.  (Severity: 4)
Expression form of "eval" at line 401, column 18.  See page 161 of PBP.  (Severity: 5)
1. Das mit "multiple" ist im Prinzip klar, ABER: das "package main;" ist nur drin, um eine 5-er-Meldung zu vermeiden, und es ist mir nicht gelungen, eine Namensgebung des packages zu generieren, die nicht beanstandet worden wäre. Insbesondere der Vorschlag von CoolTux hatte nicht funktioniert, statt FHEM 98::RandomTimer auch nicht. Das hätte aber doch funktionieren sollen, es sei denn, Ziffern wären ungültig (was ich vermute) => der/die Dateiname/n müßte/n anders werden => kann ich im Moment alleine wenig ausrichten, oder?
(=> "kleineres Übel => use main;)

2. Das "eval" habe ich in der Variante eval { expression }; getestet, aber da tut es leider nicht was es soll. Bevor ich die Meldung aber aktiv stummschalte, wollte ich mich vergewissern, dass ich nichts übersehen habe...?
Sinn dieses "eval" ist es, dem User über ein Attribut die Möglichkeit zu geben, selbst eine Funktion anzugeben, deren Auswertung dann über die weitere Funktionsweise bestimmt. Theoretisch kann da also Code stehen, der FHEM abschießen würde, das eval an sich macht daher m.E. Sinn (?). Ob man den Attributwert irgendwo vorkompiliert vorhalten könnte, wäre evtl. die Frage, aber das ganze ist für mich terra incognita...

Letzteres Thema dürften wir häufiger haben, weil dem User an vielen Stellen die Option bereitsteht, eigene Perl-Funktionen über die DEF oder Attribute anzuflanschen.

Sind die beiden weg, wären  dann noch Stufe 2-Meldungen vorhanden  .

Rückmeldung bis hierhin:
a) Die ganzen Änderungen aufgrund perlcritics sehen (jedenfalls soweit ich das im Kopf habe) in der Regel ziemlich anders aus als das, was im Rahmen des Code-Reviews als Verbesserung vorgeschlagen wurde...
b) Über manches mußte ich kurz nachdenken, aber das meiste bemängelte war in der Tat schlicht und ergreifend logisch, und ich habe sogar für die Level-3-Meldungen nur nachschauen müssen, was gemeint war. Dann war es einleuchtend und anhand des Beispiels auch für einen DAM wie mich machbar.

[SCMP77]

InternalTimer läuft im main Kontext und ruft die übergebene Funktion auch aus dem main Kontext auf. Wenn Du jetzt einfach schreibst (vereinfacht) InternalTimer(blablub,'exec',$hash) dann würde er meckern das er in main keine Funktion exec finden kann. Wenn Du aber exec exportierst dann findet sich in main eine Funktion RandomTimer_exec und die kannst Du über InternalTimer aufrufen mit InternalTimer(blablub,'RandomTimer_exec',$hash)

Ich habe das anders gelöst und das kommt ohne zusätzliche Exports aus.

Statt "exec" als String der Methode zu übergeben, gibt man Exec als Referenz an die Methode. Das sieht dann so aus:

Code Auswählen Erweitern

InternalTimer($timeStamp, \&exec, $hash );

Aus meiner Sicht sollte man - wenn möglich - immer Referenzen anstelle von Strings übergeben.

Das funktioniert auch bei Initialize. Das sieht dann so aus:

Code Auswählen Erweitern

sub Initialize {
    my ($modulData) = @_;

    $modulData->{DefFn}         = \&Define;
    $modulData->{UndefFn}       = \&Undef;
    $modulData->{DeleteFn}      = \&Delete;
    $modulData->{ReadFn}        = \&Read;
    $modulData->{ReadyFn}       = \&Ready;
    $modulData->{ShutdownFn}    = \&Undef;
    $modulData->{SetFn}         = \&Set;
    $modulData->{GetFn}         = \&Get;
    $modulData->{NotifyFn}      = \&Notify;
    $modulData->{AttrFn}        = \&Attr;
    $modulData->{AttrList}      =
                                  "SolvisName ".
                                  "GuiCommandsEnabled:TRUE,FALSE ".
                                  $readingFnAttributes;
    $modulData->{DbLog_splitFn} = \&DbLog_splitFn;

    return FHEM::Meta::InitMod( __FILE__, $modulData );

} # end Initialize

[RichardCZ]

Aus meiner Sicht sollte man - wenn möglich - immer Referenzen anstelle von Strings übergeben.

Definitiv. Für Funktionsreferenzen würde ich sogar das "wenn möglich" streichen.

Generell will man in seinen Programmen eher Referenzen übergeben, d.h. ich würde sogar weiter gehen:

Nicht:

Code Auswählen Erweitern

call_func(@tralala);

sub call_func {
    my @hopsasa = @_;

     return knoedel($hopsasa[1]); ...
}


sondern:

Code Auswählen Erweitern

call_func(\@tralala);

sub call_func {
    my $hopsasa = shift;

     return knoedel($hopsasa->[1]); ...
}


Nicht:

Code Auswählen Erweitern

call_func(%tralala);

sub call_func {
    my %hopsasa = @_;

     return knoedel($hopsasa{foo}); ...
}


sondern:

Code Auswählen Erweitern

call_func(\%tralala);

sub call_func {
    my $hopsasa = shift;

     return knoedel($hopsasa->{foo}); ...
}


Nicht nur fällt dann einigen vielleicht ein Stein vom Herzen, weil sie bislang die ganzen $@% Geschichten nerven (dann hat man es nur noch mit $ zu tun), der Code ist auch beträchtlich effizienter, weil man nicht den Stack totschei*t.

Es lungern aber - natürlich - Gefahren. Weil man nun nicht mehr Daten kopiert wie ein Wahnsinniger, sondern nur noch Pointer rumreicht, kann man die übergebenen Argumente leicher ändern (pass by Reference). Das kann manchmal gewünscht sein, manchmal nicht. Wie üblich gilt: Man sollte wissen was man tut.

[CoolTux]

So,

anbei mal eine Version, die durch die percritic durch ist und immer noch zu funktionieren scheint .

ABER: Es gibt zwei Dinge, die nicht sooo einfach sind, und eine der beiden  hat mit packages zu tun, daher stelle ich mal alle diesbezüglichen Fragen hier ein. Datei ist im Anhang, das Validierungssystem läuft mit Perl 5.26.

Angemakelt wird:

Code Auswählen Erweitern

perlcritic -3 98_RandomTimer.pm
Multiple "package" declarations at line 48, column 1.  Limit to one per file.  (Severity: 4)
Expression form of "eval" at line 401, column 18.  See page 161 of PBP.  (Severity: 5)
1. Das mit "multiple" ist im Prinzip klar, ABER: das "package main;" ist nur drin, um eine 5-er-Meldung zu vermeiden, und es ist mir nicht gelungen, eine Namensgebung des packages zu generieren, die nicht beanstandet worden wäre. Insbesondere der Vorschlag von CoolTux hatte nicht funktioniert, statt FHEM 98::RandomTimer auch nicht. Das hätte aber doch funktionieren sollen, es sei denn, Ziffern wären ungültig (was ich vermute) => der/die Dateiname/n müßte/n anders werden => kann ich im Moment alleine wenig ausrichten, oder?
(=> "kleineres Übel => use main;)

fhem.pl ist das main package. Alle anderen packges/Module befinden sich im Verzeichnis FHEM welches in der selben Ebene liegt wie fhem.pl. Daher wird allen packages welche in Modulen unterhalb FHEM/ liegen ein FHEM:: voran gestellt.

FHEM::AutoShuttersControl
FHEM::RandomTimer

Wenn man sich die Core Perlmodule an schaut sind diese auch so aufgebaut.
Net::Telnet

liegt unterhalb des Ordners Net/
/usr/share/perl5/Net/Telnet.pm

[RichardCZ]

fhem.pl ist das main package. Alle anderen packges/Module befinden dich im Verzeichnis FHEM welches in der selben Ebene liegt wie fhem.pl. Daher wird allen packages welche in Modulen unterhalb FHEM/ liegen ein FHEM:: voran gestellt.

Eigentlich...

möchte man im selben Verzeichnis wie fhem.pl vorhanden ist ein Verzeichnis "lib" haben, in fhem.pl ein use lib "lib" machen. Und alle Module unter "lib" schieben.

Falls irgendwann in Zukunft die Module FHEM::SNMP, FHEM::WOL, FHEM::RS485 ... heißen sollten - was wünschenswert wäre - dann wären die eben

fhem.pl
lib
lib/FHEM/SNMP.pm
lib/FHEM/WOL.pm
lib/FHEM/RS485.pm

Und wenn - sagen wir - ein FHEM::XXX so komplex wäre, dass es aus mehreren Modulen aufgebaut sein sollte, dann eben

lib/FHEM/XXX.pm
lib/FHEM/XXX/Zaehneputzen.pm
lib/FHEM/XXX/Naegelschneiden.pm
lib/FHEM/XXX/Rasieren.pm

Und ein gar komplexes Modul wie HomeMatic

lib/FHEM/HomeMatic.pm
lib/FHEM/HomeMatic/Wired.pm
...

könnte dann in

package FHEM::HomeMatic::Wired;

use FHEM::RS485;

machen und wir wären den ganzen Namespace Sotter los, modular, robust, wiederverwendbar und einfach göttlich. Hach *schwelg*. Aber der Weg ist das Ziel. Mal sehen was man realisieren kann.

Überdies ... wenn man einige CPAN pureperl-only module teil der FHEM Distri machen würde (also damit man sie nutzen kann, aber gleich den Usern mitliefert), dann wäre unter lib eben nicht nur

lib/FHEM.pm
lib/FHEM/...

sondern eben auch

lib/Export/Attrs.pm
lib/Net/Schlagmichtot.pm

et al.

[Beta-User]

Also...

Die erste 4/5-er-Meldung habe ich jetzt mit der von SCMP77 vorgeschlagenen Methode gekillt, weil ich dann nicht langwierig alle Vorkommen ersetzen mußte, sondern vereinfachtes testen möglich war. DANKE an der Stelle für den Schubs, auch wenn ich etwas gebraucht habe, bis (mir) klar war, was das jetzt konkret heißt (insbesondere: KEINE Quotes im Umfeld der "\&"-vercodeten Aufrufe!).

Das Package heißt daher jetzt "FHEM::98_RandomTimer".
Alles andere ginge wohl NICHT, perlcritic will schlicht den Dateinamen 1:1 haben, und der ist eben historisch wie er ist. (Und ehrlich gesagt ist mir die Frage ob "lib" oder nicht auch wurscht. Ist halt historisch so und auch nicht weiter schlimm, ist halt leicht anders wie anderswo, aber diese "Kleinigkeit" zu ändern ist m.E. irgendwo bei "Stufe 9000" .)



Für's Mitliefern von "cpan"-only Zeug gibt's übrigens auch  ein "lib"-Verzeichnis, das allerdings nur sehr wenig Inhalt hat (und um das sich betreffend der "cpan-only-"Inhalte m.E. niemand kümmert!)...


Eine Bitte dazu noch an manchen eifrigen Helfer:
Einfach die Dinge dann auch mal durch dasselbe Tool nudeln. Ich schreibe mir hier nicht völlig unnötig die Finger wund,  teste manches aus,  gebe dann die Rückmeldung, dass etwas vorgeschlagenes _nicht_ funktioniert und pappe Fassungen zum Gegenprüfen an...


Vielleicht mag jetzt jemand was zu "eval" schreiben, das Thema habe ich nämlich in verstärkter Form auch im WeekdayTimer...

Anbei nochmal die aktuellen Fassungen beider Module, wie ich sie grade am Testen habe.

[RichardCZ]

Für's Mitliefern von "cpan"-only Zeug gibt's übrigens auch  ein "lib"-Verzeichnis, das allerdings nur sehr wenig Inhalt hat (und um das sich betreffend der "cpan-only-"Inhalte m.E. niemand kümmert!)...

Habe ich gesehen, allerdings ist das lib Verzeichnis eben leider "inside-out". lib/FHEM wäre besser gewesen als FHEM/lib. Aber egal, ist jetzt erstmal so.
Ich könnte die CPAN Module im "lib" Verzeichnis maintainen, bzw. Wünschen nach Updates/Neuzugängen nachkommen.

Vielleicht mag jetzt jemand was zu "eval" schreiben, das Thema habe ich nämlich in verstärkter Form auch im WeekdayTimer...

Mach ich.

[CoolTux]

Das Package heißt daher jetzt "FHEM::98_RandomTimer".

Das würde ich nicht begrüßen. Ich habe noch kein einziges package gesehen welches Nummerisch am Beginn benannt ist. Ich würde es als Fehler sehen.

Package names are sometimes an exception to this rule. Perl informally reserves lowercase module names for ``pragma'' modules like integer and strict. Other modules should begin with a capital letter and use mixed case, but probably without underscores due to limitations in primitive file systems' representations of module names as files that must fit into a few sparse bytes.

https://www.perlmonks.org/?node=perlstyle

[rudolfkoenig]

Ich könnte die CPAN Module im "lib" Verzeichnis maintainen, bzw. Wünschen nach Updates/Neuzugängen nachkommen.

Ich bin dagegen, CPAN Module mit FHEM auszuliefern.
Im Nachhinein war es ein Fehler, sowas ueberhaupt zuzulassen, ich bin dafuer die Dateien zu entfernen.
Alt-Installationen haetten damit kein Problem, und Neu-Installationen muessten sie mit cpan (oder apt/yum/etc) installieren.

[RichardCZ]

Das würde ich nicht begrüßen. Ich habe noch kein einziges package gesehen welches Nummerisch am Beginn benannt ist. Ich würde es als Fehler sehen.https://www.perlmonks.org/?node=perlstyle

https://metacpan.org/pod/Module::Runtime

The module name syntax is, precisely: the string must consist of one or more segments separated by ::; each segment must consist of one or more identifier characters (ASCII alphanumerics plus "_"); the first character of the string must not be a digit. Thus "IO::File", "warnings", and "foo::123:_0" are all valid module names, whereas "IO::" and "1foo::bar" are not. ' separators are not permitted by this module, though they remain usable in Perl source, being translated to :: in the parser.

Der Validierungscode sieht so aus:

sub _is_string($) {
   my($arg) = @_;
   return defined($arg) && ref(\$arg) eq "SCALAR";
}

our $module_name_rx = qr/[A-Z_a-z][0-9A-Z_a-z]*(?:::[0-9A-Z_a-z]+)*/;

sub is_module_name($) {
   _is_string($_[0]) && $_[0] =~ /\A$module_name_rx\z/o;
}


FHEM::98_RandomTimer

ist folglich valide. Schön ist es nicht, aber nicht "illegal". Ich habe kein Problem damit Krücken temporär zu akzeptieren, wenn irgendwo ein konsens absehbar ist, dass es sich tatsächlich nur um temporäre Krücken und nicht die Endlösung handelt.

[Wzut]

und nicht die Endlösung handelt.

na na -> endgütige Lösung .

[RichardCZ]

na na -> endgütige Lösung .

So empfindlich? Dann darf ich erwarten, dass alle diese Module umbenannt werden?

88_ALL4000T.pm
88_HMCCUCHN.pm
88_HMCCUDEV.pm
88_HMCCU.pm
88_HMCCURPC.pm
88_HMCCURPCPROC.pm
88_IPWE.pm
88_Itach_IRDevice.pm
88_Itach_Relay.pm
88_LINDY_HDMI_SWITCH.pm
88_Timer.pm
88_VantagePro2.pm
88_WEBCOUNT.pm
88_xs1Bridge.pm
88_xs1Dev.pm

[Beta-User]

Ich bin dagegen, CPAN Module mit FHEM auszuliefern.
Im Nachhinein war es ein Fehler, sowas ueberhaupt zuzulassen, ich bin dafuer die Dateien zu entfernen.
Alt-Installationen haetten damit kein Problem, und Neu-Installationen muessten sie mit cpan (oder apt/yum/etc) installieren.

Ich sehe das ähnlich, aber bitte mit Bedacht - manche Teile sind gepflegt (der MySensors-Part, den ich aber ggf. auch irgendwie anders in die Module reinwursteln könnte, aber sicher nicht von jetzt auf gleich... Und ich wüßte nicht, ob das irgendwie (in einer aktuellen Fassung!) via cpan&Co zu bekommen wäre.)! Für andere sollte man wenigstens die betreffenden aktuellen (betroffenen) Maintainer fragen (betr. Firmata und v.a. MQTT-classic!).

Ansonsten birgt eine interne Zusatzverwaltung immer das Risiko, dass die Modulversionen nicht zueinander passen, und das ist dann eigentlich kein FHEM-Thema mehr (mAn.).


Was die Benennung angeht, bin ich schmerzbefreit und fände ohne den Ziffernpräfix auch "schöner" (=> "Muh" lokal abschalten?). Aber "Mama Muh" will es so, dann soll sie bekommen, nach was ihr ist - schließlich muß ich die PBP-Regeln ja eigentlich nicht in jedem Detail verstehen, und allen anderen kann es egal sein, solange ich sie befolge und der Code am Ende macht, was er soll, oder...? 

ABER:
ENTWEDER das Modul stellt Funktionen bereit, die auch andere nutzen/ansprechen können sollen, ODER eben nicht (so ist es in diesem Fall! (fast...!) .

Im Zweiten Fall ist der Name schniepswurstegal. Wer trotzdem meint, unbedingt eine Funktion nutzen zu wollen, muß eben mit dem Risiko leben, dass ich das heute so benenne und morgen anders. Er hat sich diesem Risiko bewußt ausgesetzt, diese Funktionen sind ja gerade deswegen gekapselt, weil wir Irrungen und Wirrungen vermeiden wollen, oder?

Sollen doch AUSNAHMSWEISE andere Module Funktionen aus dem Package nutzen können, gibt es zwei Wege, auf deren Nutzung man sich verständigen sollte:
1. Man bietet einen "set" bzw. "get" an. Gibt's sowas, ist diese Methode zu nutzen. Nicht ganz so effizient, aber es gilt im Prinzip dasselbe, was Oli in dem anderen Thread zu Attributen geschrieben hat: Das ist aus guten Gründen eine Konvention.
2. Soll ausnahmsweise der User davon nichts mitbekommen, dass ein Modul eine API hat, dann (und nur dann!) sollte ein Export stattfinden. Dann hat der Modulautor aber bitte auch darauf zu achten, dass der Name konstant bleibt...

(Nochmal: Das wäre ein erster _Vorschlag_ für eine Konvention, auf die man sich vielleicht verständigen sollte.
Beachte: ich bin DAM, und kein hauptberuflicher Entwickler (weder Perl noch irgendeine andere Programmiersprache) oder IT-Architekt, es kann also sein, dass das mal wieder viel zu kurz gegriffen ist...).

[Beta-User]

So, ein paar Iterationen weiter...

- Habe jetzt doch die "GP_Export"-Variante eingebaut. Man kann das zwar auch "zu Fuß" machen, aber was auch immer ich gestern vermeintlich getestet hatte, "einfach so" mag fhem.pl die "Initalize"-Funktion nicht finden (@Rudi: da wäre evtl room for improvement?).
- Da "eval" in der vorliegenden Form benötigt wird, und mir noch keiner gezeigt hat, wie man Funktionen aus Attributen ggf. irgendwo vorkompiliert ablegen könnte(?), ist an der Stelle eben perlcritic auf "stumm" geschaltet.
- Und wenn ich schon am "Tricksen" bin, was perlcritic angeht, habe ich das gleich noch für den package name übernommen, so böse kommt mir "das foul" an der Stelle nicht vor...

- daneben sind mir beim Durchsehen des codes auch noch ein paar weitere Kleinigkeiten aufgefallen, der angehängte Patch ist daher kein reiner "perlcritic => <3-patch" (das sind aber nur wenige Zeilen, die was anderes betreffen...!)

Werde das jetzt nochmal bei mir ein paar Tage testen und ggf. dann auch über den "RT-Thread" zum testen anbieten, bevor ich es einchecke...

Grüße und Danke für den support!

PS:
- den WeekdayTimer habe ich auch etwas intensiver angesehen, da sind noch ein paar interessante Aspekte drin, für die ich aber beizeiten dann mal einen gesonderten Thread aufmache, da ich den definitiv vorerst nicht als package umbauen werde und das dann auch ganz andere, nicht package-related Dinge sind. (ein paar "eval" sind da aber drin, derer ich (noch) nicht Herr geworden bin).
- Die MySensors-Geschichte ist dann noch eine Baustelle, auch das wird zu gegebener Zeit nochmal Thema werden, nehme ich mal an...