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.

[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...

[RichardCZ]

@Beta-User - dieses no warnings smartmatch-operator ... das ist notwendig oder ist das ein Überbleibsel? Ich konnte jetzt auf den ersten Blick nirgends eine Verwendung von ~~ entdecken.

[Beta-User]

Zu finden in der sub Attr ab Zeile 177. (Oder habe ich die Frage falsch interpretiert?)

Ich hänge auch mal eine etwas aktualisierte Fassung vom WDT an, da wir grade dabei sind...
Da gibt es auch 4 "eval"-Warnungen.

- Über Zeile 461 denke ich grade nach, bin aber noch nicht zu einem Ergebnis gekommen, was das eigentlich für einen Zweck hat.

- Zeilen 514 und 525 sehen mir danach aus, also könnte das ein Fall für die Umwandlung in unbenannte sub's sein, aber sicher bin ich mir nicht.

=> Vielleicht kann du als Experte das jeweils auf einen Blick beantworten, bevor ich lange rumteste?

- Zeile 1015 sieht mir wie ein Wiedergänger von der RT-Problematik aus, hier allerdings noch "garniert" durch die Ersetzung typischer Parameter. => Plaster?

Ggf. hast du damit dann auch etwas mehr "Spielmaterial" für einen Vorschlag für eine "zentrale eval-Alternative"...?

[rudolfkoenig]

Die "zentrale eval-Alternative" heisst AnalyzePerlCommand.
Der Haken: neben eval macht es etliche potentiell ueberfluessige Sachen ($hour,$we etc setzen, Variablen wie EVTPARTx zu uebergeben), ist also langsamer, als ein einfaches eval.
Das Gute: wenn das eval ein WARNING generiert, findet man mit stacktrace raus, wo das Problem aufgetreten ist, und was gerade dabei evaluiert wurde.

[RichardCZ]

Zu finden in der sub Attr ab Zeile 177. (Oder habe ich die Frage falsch interpretiert?)

Uh - ne ich seh's schon.

Code (perl) Auswählen Erweitern

sub Attr {
    my ($cmd, $name, $attrName, $attrVal) = @_;

    my $hash = $defs{$name};

    if ($attrName eq 'switchmode') {
        setSwitchmode($hash, $attrVal);
    }

    if ($attrName =~ m{\A disable(Cond)? \z}xms) {
        # Pull the schaltening before, because offturning wanted when disable instead of teutonian comments
        RmInternalTimer("Exec", $hash);
        MkInternalTimer("Exec", time()+1, \&Exec, $hash, 0);
    }
   
    if ($attrName eq 'offState') {
        my ($offRegex, $offReading) = split ' ', $attrVal, 2;
        $hash->{helper}{offRegex}   = $offRegex;
        $hash->{helper}{offReading} = $offReading // 'state';
    }
 
    return;
}


Dann sollte die nowarnings smartwatch Geschichte auch wegkönnen.

Ich hänge auch mal eine etwas aktualisierte Fassung vom WDT an, da wir grade dabei sind...
Da gibt es auch 4 "eval"-Warnungen.

- Über Zeile 461 denke ich grade nach, bin aber noch nicht zu einem Ergebnis gekommen, was das eigentlich für einen Zweck hat.

Was der Code für einen Sinn hat, habe ich auch noch nicht begriffen, aber da sollte jedenfalls ein Block-Eval gehen,

- Zeilen 514 und 525 sehen mir danach aus, also könnte das ein Fall für die Umwandlung in unbenannte sub's sein, aber sicher bin ich mir nicht.

Sinn wieder mal dahingestellt, auch das geht mit einem Block-Eval (Compilezeit)

- Zeile 1015 sieht mir wie ein Wiedergänger von der RT-Problematik aus, hier allerdings noch "garniert" durch die Ersetzung typischer Parameter. => Plaster?

Ggf. hast du damit dann auch etwas mehr "Spielmaterial" für einen Vorschlag für eine "zentrale eval-Alternative"...?

Also WeekdayTimer_FensterOffen ist eine richtige Baustelle. Es würde mir helfen, wenn ich ein paar Beispiele sehen würde für den Inhalt von $verzoegerteAusfuehrungCond bevor irgendwas damit gemacht wird. Also schon zu dem Zeitpunkt an dem es aus
AttrVal($hash->{NAME}, "delayedExecutionCond", "0");
geholt wurde.

Die "zentrale eval-Alternative" heisst AnalyzePerlCommand.
Der Haken: neben eval macht es etliche potentiell ueberfluessige Sachen ($hour,$we etc setzen, Variablen wie EVTPARTx zu uebergeben), ist also langsamer, als ein einfaches eval.
Das Gute: wenn das eval ein WARNING generiert, findet man mit stacktrace raus, wo das Problem aufgetreten ist, und was gerade dabei evaluiert wurde.

Unabhängig von der kleinen Performance Penalty sollte man es dann trotzdem verwenden. So fehlt z.B. in dem o.g. WDT Code an Zeile 1015 jedwede $@ Behandlung bzw. ein Abfangen potentieller Fehler. Somit ist das eine Stelle, wo man FHEM hart "zur Landung zwingen" könnte.

[Beta-User]

Hmm, also für die RT-eval-Geschichte sähe das dann mit AnalyzePerlCommand so aus, oder?
(hätte ich ja auch selbst drauf kommen können..., Analy.*muß dann auch nach GP_Import).

Code Auswählen Erweitern

sub isDisabled {
   my $hash = shift;

   my $disable = AttrVal($hash->{NAME}, "disable", 0 );
   return $disable if($disable);

   my $disableCond = AttrVal($hash->{NAME}, "disableCond", "nf" );
   if ($disableCond eq "nf") {
     return 0;
   }
   
   return AnalyzePerlCommand($hash, $disableCond);
   
   ##seitherige Fassung auskommentiert:
   #else {
   #   $disable = eval ( $disableCond ); ## no critic 'eval' # not working when written as eval  { $disableCond };
   #   if ($@) {
   #      $@ =~ s/\n/ /gx;
   #      Log3 ($hash, 3, "[$hash->{NAME}] ERROR: " . $@ . " EVALUATING " . $disableCond);
   #   }
   #   return $disable;
   #}
}

Wenn das da klappt, sollte es dann 1:1 auch für den WDT zu verwenden sein, von daher sehe ich mal davon ab, dazu ein Beispiel zu liefern.

Das mit dem smartmatch-Ersatz folgt dann noch, und da und vorallem auch bei dem Block-call fehlt mir noch der Durchblick, wie ich das ggf. sinnvoll testen könnte...
Vermutlich macht es am meisten Sinn, das "für Mutige" zum Testen anzubieten, aber bei meinen diesbezüglichen Anfragen (auch für die wenigen neuen features da) war in der Vergangenheit wenig feedback zu bekommen. Ggf. würde ich das - wenn fertig und ggf. auch nochmal von euch kritisch beäugt - auch "auf Verdacht" ins svn schieben.

ABER (bzgl. direkter Veröffentlichung@svn): Es wäre nicht sooo toll, wenn wir hier Aufräumarbeiten anschieben und das erste, was die User davon sehen, wären dann unmotivierte Abstürze oder unerklärliche neue Verhaltensweisen. Für den Fall dass, hoffe ich auf viele Helfer, die schnell mit reparieren...

(@CoolTux: Das "Pflaster" beim Package-Namen darf bleiben, auch wenn ich mich damit vermutlich für die "Hall of fame" disqualifiziere . Und wenn es dann läuft, machen wir ggf. auch noch eine "Schönheitoperation" betr. die Optik, wenn es denn sein muß. Aber wir beschränken uns dann darauf und trennen es strikt von funktionalen Themen .)

Wenn wir hier beim RT durch sein sollten, würde ich mich ggf. direkt noch an WDT machen, ABER: auch das Teil ist ein "Erbstück", das schon durch ein paar Hände gegangen ist, und die verbliebenen "Level-3-Muher" kann ich zwar nachvollziehen, aber die zu lösen ist ungleich schwieriger als das Löschen von ein paar Prototypes...

[Wzut]

@Beta-User, warum 

Code Auswählen Erweitern

my $disable = AttrVal($hash->{NAME}, "disable", 0 );
   return $disable if($disable);
und nicht :

Code Auswählen Erweitern

return if (AttrNum($hash->{NAME}, "disable", 0 ));

BTW : Mit dem Attr disable habe ich früher auch nur so gemacht, bis mir Module unter kamen die zusätlich noch ein set disable unterstützen.
Vorteil : mit dem set kann man auch mal eben schnell temporär das Modul lahmlegen ohne config Änderung (Stichwort rotes Fragezeichen).
I.d.R. zieht das set <name> disable auch gleich state mit , das ReadingsVal( $hash->{NAME}, "state", 'disabled' ) müsste dann natürlich oben noch als oder dazu.
 

[Beta-User]

...das scheint jedenfalls bei ersten Tests zu klappen, anbei daher nochmal eine aktualisierte Fassung samt Gesamt-patch...

Hoffe, so wird ein Schuh draus, und an diesem Muster können sich ggf. dann auch andere orientieren.

Die elegantere Fassung mit

Code Auswählen Erweitern

return if (AttrNum($hash->{NAME}, "disable", 0 ));ist eingebaut, über den 2. Aspekt muß ich bei Gelegenheit mal nachdenken. Ist zwar doof, dass man das disable bei diesen alten Modulen teils nur via Attribut setzen kann, aber hier ist es so, dass sich der RT ja je nach Attributauswertung auch auf "disable" setzt. Fürchte, da wird dann eine unbeabsichte (logische) "Schleife" an der falschen Stelle geschlossen (?). Wenn ich da aber für die "psitive Auswertung" des Attributs jetzt mit einem anderen "state"-Inhalt reinfunke, ist das Risiko groß, dass sich da direkt jemand beschwert, weil er das irgendwie auswertet... (?)

[rudolfkoenig]

Die elegante Version ist mAn ist IsDisabled($hash->{NAME}), sie prueft auch nach disabledForIntervals.

[Beta-User]

Die elegante Version ist mAn ist IsDisabled($hash->{NAME}), sie prueft auch nach disabledForIntervals.

Kurze Rückmeldung dazu:

Nette Idee, ich werde versuchen, das feature einzubauen.
Das hat aber einen "kleinen" Haken: Auch die Evaluierung der DisableCondition führt zu einem "inactive" (sofern zutreffend) oder einer Aktivierung für diesen Tag bei Tageswechsel. Irgendwie muß man daher m.E. die Fälle auch dauerhaft auseinanderhalten können, jedenfalls, wenn man bei der Gelegenheit auch noch ein "set xy inactive" bzw. active zulassen möchte und disableForInternals nicht dazu führen soll, dass wirklich nur für den Zeitraum "still" ist... Will heißen: Dauert noch etwas, ich muß das gedanklich erst nochmal in den ganzen Varianten durchspielen. Vermutlich muß ich ein Reading generieren, ob (nur) die Evaluierung inactive ergibt oder ob es eine User-Anweisung ist (=dauerhaft, bis zur willentlichen Wieder-Aktivierung).