98_RandomTimer.pm Code Review und packages

[CoolTux]

Hallo Jörg,

Ich baue gerade das Modul um auf packages. Ist nicht ganz so einfach da FHEM Funktionen mit eigenen Namen verwendet werden. InternalTimer und RemoveInternalTimer. Da muss man höllisch auf passen.
Das ganze läuft aber schon ganz gut soweit. Habe auch einige Logische Fehler schon ausmerzen können.
Zum Beispiel wurde in der Define Routine RandomTimer_RemoveInternalTimer sehr früh aufgerufen, in der Funktion RandomTimer_RemoveInternalTimer wird dann versucht $hash->{NAME} aus zu lesen, aber zu diesem Zeitpunkt ist $hash-{NAME} noch gar nicht definiert. Das $hash->{NAME} = = $name; kommt erst nach dem RandomTimer_RemoveInternalTimer aufruf in der Define Funktion.


Grüße

[CoolTux]

Zum Beispiel wurde in der Define Routine RandomTimer_RemoveInternalTimer sehr früh aufgerufen, in der Funktion RandomTimer_RemoveInternalTimer wird dann versucht $hash->{NAME} aus zu lesen, aber zu diesem Zeitpunkt ist $hash-{NAME} noch gar nicht definiert. Das $hash->{NAME} = = $name; kommt erst nach dem RandomTimer_RemoveInternalTimer aufruf in der Define Funktion.

Ok vergiss den Teil. Da hat sich tatsächlich eine einzige FHEM RemoveInternalTimer Funktion eingeschlichen. Sorrymein Fehler

[Beta-User]

...was den Rest angeht:

10_MYSENSORS_DEVICE.pm löst das Import-Problem so:

Code Auswählen Erweitern

BEGIN {
    main::LoadModule("MYSENSORS");
    MYSENSORS->import(qw(:all));

  GP_Import(qw(
    AttrVal
    readingsSingleUpdate
    readingsBeginUpdate
    readingsEndUpdate
    readingsBulkUpdate
    CommandAttr
    CommandDeleteAttr
    CommandDeleteReading
    AssignIoPort
    Log3
    SetExtensions
    SetExtensionsCancel
    ReadingsVal
    ReadingsNum
    InternalVal
    FileRead
    InternalTimer
    RemoveInternalTimer
  ))
};


Kann leider nicht sagen, ob das "best practice" ist, aber es funktioniert...

[CoolTux]

Wenn Du magst kannst Du gerne mal drüber schauen und testen.

https://git-tuxnet.ddns.net/FHEM/mod-RandomTimer


Beim Anlegen kommt noch eine Meldung von wegen "Timername schon vorhanden erst löschen"

[Beta-User]

Erst mal beim drüberschauen Fragen eines DAM:

- Warum läßt du "Initialize" nicht außerhalb des Packages? Das dann wieder zu Exportieren ist an der Stelle m.E. etwas "too much", das Modul funktioniert ja sowieso nur im FHEM-Kontext richtig. Und würden sich dann nicht die diversen Initialize-Funktionen irgendwie überschreiben, wenn das alle Module so machen?
- Warum auch "exec" exportiert wird, ist mir unklar; ohne das im Detail verstanden haben zu wollen,  kommt es mir sogar so vor, als würde das das Risiko erhöhen, dass irgendwer weitere exec's definiert und dann gerade nicht mehr klar ist, welches denn gemeint ist...?

Den Rest muß ich mir mal als diff ansehen, Basis war aber die Fassung vor meinem commit ins svn, richtig?


NACHTRAG:

Die "Initalize"-Funktion scheint ja was FHEM-spezifisches zu sein. Wäre es dann nicht besser, fhem.pl dahingehend zu erweitern, dass das nicht nur "<Modulname>_Initialize()" sucht, sondern auch "<Modulname>::Initialize()", um die enthaltenen Funktionen ggf. sauber ins "Ökosystem" einzubinden?

(Kurz: das mit dem Export leuchtet mir jedenfalls in der Form als DAM nicht ein...).

[CoolTux]

Erst mal beim drüberschauen Fragen eines DAM:

- Warum läßt du "Initialize" nicht außerhalb des Packages? Das dann wieder zu Exportieren ist an der Stelle m.E. etwas "too much", das Modul funktioniert ja sowieso nur im FHEM-Kontext richtig. Und würden sich dann nicht die diversen Initialize-Funktionen irgendwie überschreiben, wenn das alle Module so machen?
- Warum auch "exec" exportiert wird, ist mir unklar; ohne das im Detail verstanden haben zu wollen,  kommt es mir sogar so vor, als würde das das Risiko erhöhen, dass irgendwer weitere exec's definiert und dann gerade nicht mehr klar ist, welches denn gemeint ist...?

Das ist einfach beantwortet. Die Export Funktion setzt vor jeder Funktion ein Präfix. Aus Initialize wird PACKAGENAME_Initialize also in unserem Fall RandomTimer_Initialize. Und genau das war auch der Grund, warum ich exec exportiert habe. Allerdings war das ganz am Anfang, bis ich mitbekommen habe das es da noch mehr Aufrufe gibt. Grund ist folgender.

InternalTimer(blablub,'FN_Aufruf_aus_Mainfuntion_heraus)

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)

Das ist schon das ganze Geheimnis. Und genau so funktioniert auch das exportieren der Initialize. Aus einem einfachen Initialize wird RandomTimer_Initialize.
Das ich später die InternalTimer nicht mit RandomTimer_*** aufrufe ist meine Schuld, ich habe den Packageaufruf gewählt. Von daher kann wie Du schon festgestellt hast das exec auch raus aus den Export.

Den Rest muß ich mir mal als diff ansehen, Basis war aber die Fassung vor meinem commit ins svn, richtig?


NACHTRAG:

Die "Initalize"-Funktion scheint ja was FHEM-spezifisches zu sein. Wäre es dann nicht besser, fhem.pl dahingehend zu erweitern, dass das nicht nur "<Modulname>_Initialize()" sucht, sondern auch "<Modulname>::Initialize()", um die enthaltenen Funktionen ggf. sauber ins "Ökosystem" einzubinden?

(Kurz: das mit dem Export leuchtet mir jedenfalls in der Form als DAM nicht ein...).

Das wäre sicherlich auch ein Weg. Müsste ich mir mal anschauen.

[CoolTux]

Ich bin dann fertig. Habe den Fehler mit der Meldung das der Timer schon existiert gefunden.
Kannst Dir jetzt die fertige Fassung anschauen.
Achtung es kann sein das noch Funktionen aufgerufen werden die ich noch nicht import habe. Hoffe aber jetzt alle erwischt zu haben. Ich teste aber.

[Beta-User]

Hmmm, habe mir das jetzt mal überschlägig angesehen.

Habe vorab mal Anmerkungen aus drei Themenkreisen dazu:

1. GP_Utils - Import/Export an sich.
Das sind Funktionen, die afaik ntruchsess mal eingeführt hatte (der ursprüngliche Maintainer der MySensors- und MQTT-Module), und zwischenzeitlich nur von einigen wenigen anderen Modulen mit genutzt werden.

a) Wenn wir das jetzt in größerem Stil empfehlen wollten, wäre für mich die Frage, ob
- die Funktionen am richtigen Ort sind, oder zukünftig dann nicht direkt in fhem.pl reingehören;
- die Funktionalität an sich "gut" ist, denn diese Frage ist m.E. noch nicht wirklich beantwortet:

Kann leider nicht sagen, ob das "best practice" ist, aber es funktioniert...

.

b) Norbert war damals selbst eher zurückhaltend mit dem exportieren, soweit ich das beurteilen kann.
Dem Bauchgefühl nach würde ich auch weiter dazu tendieren, das für eine "gute Praxis" anzusehen und gerade im Interesse des "Freihaltens" des Namespace auch weiter so zu halten. Folglich läge es m.E. näher
- insbesondere darauf zu verzichten, "exec" zu exportieren und main::InternalTimer dann eben so aufzurufen, dass die "Package-Adresse" mit übergeben wird. Ist etwas mehr Schreibarbeit, aber kein Beinbruch, und mMn. für andere einfacher zu verstehen.
- Aus demselben Grund würde ich - jedenfalls im Moment - das "Initialize" auch weiter außerhalb des Package halten.

2. Das FHEM::RandomTimer scheint mir eine allg. Perl-Konvention zu sein, aus der sich ergibt, dass das Package nur im FHEM-Kontext Sinn macht. Wenn das so ist, ok, ansonsten - sollte man die bisherige Verteilung über den internen update-Mechanismus beibehalten - nehme ich das (provokativ formuliert ) eher als "überflüssige Schreibarbeit" wahr.

3. Meine eigentliche Intention war ja _auch_:

Code Auswählen Erweitern

Das wäre ggf. für weitere Maintainer als gutes Beispiel hilfreich? der RT-Code ist ja nicht besonders umfangreich...

Wenn ich mir jetzt aber das diff ansehe, findet sich darin "alles mögliche", über das man scheinbar teils heftig streiten kann, und das teils dann auch nicht konsequent zu Ende geführt ist (z.B. ist der Prototype in GetHashIndirekt noch drin).

Wenn man das ganze als showcase für andere nutzen will, dann sollte man diese Teile sauber abschichten und je in ein eigenes diff packen:
- allg. Formatierung
  -- (was stört dich an 2 Leerzeichen statt 4?, was ist an einfachen Quotes besser als an doppelten (nicht der allg. Unterschied, sondern: was ist der tiefere Sinn der Ersetzung an den konkreten Stellen, z.B. gleich zu Beginn?))
  -- Leerzeichen um Klammern (kann sein, dass das besser lesbar ist)
  -- Textumbrüche und concats - wo aufbrechen, wo nicht...? (Das scheint mir nach wie vor nicht konsistent zu sein! Jedenfalls sind da teils noch explizite contatenations drin, die man so eigentlich gar nicht (mehr?) braucht, sondern direkt die Variablennamen in den Text aufnehmen könnte(?).)
  -- Welche Klammern überhaupt (?, ist da berücksichtigt, was Richard an anderer Stelle dazu geschrieben hatte? (Eher nicht, zumindest hatte ich auf die Schnelle ein "split" ausfindig gemacht....)

Da müsste man natürlich auch informiert sein und wissen warum bei BUILTINS (nicht Funktionen) die Argumente bevorzugt nicht mit Klammern angegeben werden.

- PBP - konformere Schreibweisen
(s.o.)

- Entfernung von prototypes

- das eigentliche "Package"-Thema.


Ich mache mir gerne die Mühe, das aufzubereiten (und dann step-by-step auch ins svn einzuchecken, damit man die Schritte je für sich nachvollziehen kann!) und ggf. sogar konkrete "Bibelzitate" einzufügen, wenn das weiterhilft (zusammensuchen oder gar erraten will ich das aber möglichst nicht!), aber mit diesem (im Vergleich zum Gesamtcode) riesigen "Mischmasch-Diff" motiviert man m.E. keinen, sich mit Packages auseinanderzusetzen, sondern es entsteht der (m.E. falsche!) Eindruck, das sei ein Haufen Arbeit, der am Ende nichts bringt, und durch den nur die Vorlieben eines Coders durch die eines anderen ersetzt werden.

Das wäre doch schade, oder?

[RichardCZ]

GPUtils (wow - GPU Support  )  habe ich mir angesehen. Ich klassifiziere das eher als redlichen Versuch eine Unzulänglichkeit (Namespace Gewusel) so gut zu kaschieren wie es eben geht. Über GPUtils bin ich gestolpert, als ich das 73_AutoShuttersControl "eval/require/import Massaker" erwähnt habe und ein Snippet dazu gepostet habe:

https://gl.petatech.eu/root/HomeBot/snippets/2

GPUtils macht in wesentlichen Grundzügen sowas plus natürlich noch einiges mehr um für die Devs einen gewissen Komfort zu erreichen.
Also um es ganz klar zu sagen: GPUtils bzw. eine prozedurale Herangehensweise um Module zu laden, irgendwas in den Namespace zu importieren, ... ist immer noch besser als besagtes "Massaker", aber letztlich doch nur ein Notnagel.

Ich würde bis zum Erscheinen eines "richtigen neuen Mustermoduls" nicht unendlich Energie in solche Umbauarbeiten investieren. Das ist eventuell unnötige Mehrarbeit und es ist ja nicht so, dass anderweitig Mangel an Arbeit wäre.

[CoolTux]

Hmmm, habe mir das jetzt mal überschlägig angesehen.

Habe vorab mal Anmerkungen aus drei Themenkreisen dazu:

1. GP_Utils - Import/Export an sich.
Das sind Funktionen, die afaik ntruchsess mal eingeführt hatte (der ursprüngliche Maintainer der MySensors- und MQTT-Module), und zwischenzeitlich nur von einigen wenigen anderen Modulen mit genutzt werden.

a) Wenn wir das jetzt in größerem Stil empfehlen wollten, wäre für mich die Frage, ob
- die Funktionen am richtigen Ort sind, oder zukünftig dann nicht direkt in fhem.pl reingehören;
- die Funktionalität an sich "gut" ist, denn diese Frage ist m.E. noch nicht wirklich beantwortet:.

b) Norbert war damals selbst eher zurückhaltend mit dem exportieren, soweit ich das beurteilen kann.
Dem Bauchgefühl nach würde ich auch weiter dazu tendieren, das für eine "gute Praxis" anzusehen und gerade im Interesse des "Freihaltens" des Namespace auch weiter so zu halten. Folglich läge es m.E. näher
- insbesondere darauf zu verzichten, "exec" zu exportieren und main::InternalTimer dann eben so aufzurufen, dass die "Package-Adresse" mit übergeben wird. Ist etwas mehr Schreibarbeit, aber kein Beinbruch, und mMn. für andere einfacher zu verstehen.
- Aus demselben Grund würde ich - jedenfalls im Moment - das "Initialize" auch weiter außerhalb des Package halten.

Norbert blieb ja auch keine andere Wahl als der Zurückhaltung beim Exportieren. Die Export Funktion gab es damals nämlich gar nicht. Es gab lediglich die Import Funktion. Julian und ich haben die Exportfunktion erst vor einem halben oder einem Jahr eingebaut wie ich die Maintainerschaft für das Modul übernommen habe.

Ich würde das als Modul lassen.

2. Das FHEM::RandomTimer scheint mir eine allg. Perl-Konvention zu sein, aus der sich ergibt, dass das Package nur im FHEM-Kontext Sinn macht. Wenn das so ist, ok, ansonsten - sollte man die bisherige Verteilung über den internen update-Mechanismus beibehalten - nehme ich das (provokativ formuliert ) eher als "überflüssige Schreibarbeit" wahr.

3. Meine eigentliche Intention war ja _auch_:

Code Auswählen Erweitern

Das wäre ggf. für weitere Maintainer als gutes Beispiel hilfreich? der RT-Code ist ja nicht besonders umfangreich...

Wenn ich mir jetzt aber das diff ansehe, findet sich darin "alles mögliche", über das man scheinbar teils heftig streiten kann, und das teils dann auch nicht konsequent zu Ende geführt ist (z.B. ist der Prototype in GetHashIndirekt noch drin).

Wenn man das ganze als showcase für andere nutzen will, dann sollte man diese Teile sauber abschichten und je in ein eigenes diff packen:
- allg. Formatierung
  -- (was stört dich an 2 Leerzeichen statt 4?, was ist an einfachen Quotes besser als an doppelten (nicht der allg. Unterschied, sondern: was ist der tiefere Sinn der Ersetzung an den konkreten Stellen, z.B. gleich zu Beginn?))
  -- Leerzeichen um Klammern (kann sein, dass das besser lesbar ist)
  -- Textumbrüche und concats - wo aufbrechen, wo nicht...? (Das scheint mir nach wie vor nicht konsistent zu sein! Jedenfalls sind da teils noch explizite contatenations drin, die man so eigentlich gar nicht (mehr?) braucht, sondern direkt die Variablennamen in den Text aufnehmen könnte(?).)
  -- Welche Klammern überhaupt (?, ist da berücksichtigt, was Richard an anderer Stelle dazu geschrieben hatte? (Eher nicht, zumindest hatte ich auf die Schnelle ein "split" ausfindig gemacht....)- PBP - konformere Schreibweisen
(s.o.)

- Entfernung von prototypes

- das eigentliche "Package"-Thema.


Ich mache mir gerne die Mühe, das aufzubereiten (und dann step-by-step auch ins svn einzuchecken, damit man die Schritte je für sich nachvollziehen kann!) und ggf. sogar konkrete "Bibelzitate" einzufügen, wenn das weiterhilft (zusammensuchen oder gar erraten will ich das aber möglichst nicht!), aber mit diesem (im Vergleich zum Gesamtcode) riesigen "Mischmasch-Diff" motiviert man m.E. keinen, sich mit Packages auseinanderzusetzen, sondern es entsteht der (m.E. falsche!) Eindruck, das sei ein Haufen Arbeit, der am Ende nichts bringt, und durch den nur die Vorlieben eines Coders durch die eines anderen ersetzt werden.

Das wäre doch schade, oder?

GetHashIndirekt habe ich dann wohl übersehen 

[Beta-User]

So, jetzt wollte ich es trotzdem wissen, wie ein "Minimal-Patch" aussehen "könnte", wenn man Packages verwenden will.

Ein paar Anmerkungen:
- das mit GPUtils gefällt mir nur bedingt, irgendwie ist das gefühlt ein Fremdkörper. Daher habe ich das nur für den Import verwendet, weil mir auch nichts besseres eingefallen ist, um Dinge aus dem main-Namespace zu übernehmen, die man halt im FHEM-Kontext braucht.
Wenn da wer Tipps hat, wie man das "besser" machen kann: feel free. (Das mit "our" habe ich schon registriert, aber das läge außerhalb der Zuständigkeit des DAM, oder?)

- M.E. sollte man package-interne Funktionen nicht so benennen wie externe, das führt sonst m.E. früher oder später zu Verwirrung - hier war es "InternalTimer", das ist daher die einzige Funktion, die in dem Zusammenhang einen wirklich anderen Namen erhalten hat. Der Rest ist search/replace, nur bei den Funktionen, die aus main heraus wieder aufgerufen werden sollen (hier eigentlich nur via InternalTimer), muß man halt mit den entsprechenden Package-Präfix versehen. (Dass CoolTux das in der letzten Iteration anders vorgeschlagen hatte, habe ich gesehen, aber wie gesagt: das mit dem Export ist mir suspekt, und man braucht es für modulinterne Dinge nicht wirklich).

Wer mag, kann und darf natürlich das fertige Ergebnis testen, ich checke das bei Gelegenheit ein, und werde mir auch die anderen Vorschläge zur Code-Überarbeitung zu Gemüte führen.


Und ja: Ich habe wahrgenommen, dass der Tenor grundsätzlich war, man würde diese ganzen Zwischenschritte nicht brauchen. Ich bitte um Verständnis, dass ich hier die Entscheidung getroffen habe, nicht den einfachen Weg zu gehen und einfach nur was mehr oder weniger blind zu übernehmen, sondern diese Zeit aufzuwenden.

Kann ja durchaus sein, dass das jemand irgendwie hilfreich findet, der einfach nur diese EINE Sache mit den Packages "wissen" will.

Gruß, Beta-User

[CoolTux]

Du kannst Funktionen und Variablen aus der Main auch einfach

::FnNAME

oder

main::FnName

aufrufen.
Im Grunde brauch man die Import und Export Geschichten nicht.

[Beta-User]

Danke für den Hinweis.

Nach meinem derzeitigen "Gefühlsstand" würde ich sagen, dass es im Sinne einer Diskussionsgrundlage als "Beispiellösung für potentielle Umsteiger" einfacher ist, zu importieren, was man baucht. Das ist nicht nur für den Umstieg einfacher, weil man nicht alle Stellen im Code suchen muß, an denen das betreffende Element im Code auftaucht, man macht sich (und anderen) dadurch dauerhafter bewußt, was man eigentlich von main "borgt" - es steht an einer Stelle und nicht irgendwie verstreut im Code.

Aber wir können das gerne diskutieren, genauso wie die Frage, ob diese Art der Umstellung im Moment überhaupt Sinn macht.

Nochmal:

3. Meine eigentliche Intention war ja _auch_:

Code Auswählen Erweitern

Das wäre ggf. für weitere Maintainer als gutes Beispiel hilfreich? der RT-Code ist ja nicht besonders umfangreich...

Wenn das irgendwie kontraproduktiv sein sollte, weil es eben kein gutes Beispiel ist, würden mich die Argumente interessieren...

(Und dass man als Entwickler vielleicht vorrangig einen Blick auf diese Dinge hier werfen sollte (Perl::Critic (https://metacpan.org/pod/Perl::Critic) und Module::CoreList (https://metacpan.org/pod/Module::CoreList)), soll auch gar nicht mit diesem Thread hier in Frage gestellt werden).

[RichardCZ]

Nach meinem derzeitigen "Gefühlsstand" würde ich sagen, dass es im Sinne einer Diskussionsgrundlage als "Beispiellösung für potentielle Umsteiger" einfacher ist, zu importieren, was man baucht.

Ja. Wobei ich gerne noch was schreiben möchte über Export/Import und was es da an guten Lösungen gibt. Bitte aber um Geduld - geht leider nicht alles von heute auf morgen und will auch keinen mit der Informationsfülle erschlagen (ist so schon bestimmt grenzwertig).

[CoolTux]

Kleine Anmerkung noch von mir zum Thema Variablen

$::defs

So ruft man dann Variablen aus der main auf.