-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load Protocolhash from PerlModule instead from textfile #522
Conversation
Ich hab mal was probiert. Es klappt aber irgendwas beim Syntaxcheck der Module nicht :( |
Sehe ich das richtig, du möchtest das selbige wie Ralf machen, nur in einem eigenem Package? |
Mich würde es wundern, wenn es so funktionieren würde. |
@HomeAutoUser @Ralf9 Bin jetzt nicht ganz sicher was Du mit "Variante von Rudi und betateilchen" genau meinst. Es im Package main zu belassen? Oder Die Variante mit "use" zu welcher ich folgendes in der Perldoku finde "the use of this pragma is discouraged" ? |
So, wenn man das Perl Modul nicht in die Perl Installation kopiert, dann kann es auch nicht funktionieren .. Jetzt schlagen all die Tests fehlt, in denen eine andere Protocol Hash Datei geladen wird. Das habe ich mir bereits gedacht. Die müssten auch umgebaut werden. |
Ich brauche keine unittests laufen lassen um zu sehen ob es funktioniert. |
Keine Ahnung worum es jetzt geht. Was genau bei welcher Variante funktioniert und was nicht ist mir nicht transparent. Daher habe ich es halt mal ausprobiert. Die Variante, ein Package zu erstellen und darin eine Methode zum holen der Daten zu platzieren, schien mir von den mir bekannten am besten geeignet. Das initiale und einmalige laden des Packages ist im übrigen auch kein Problem. Ein bisschen tricky wird es, wenn man eine andere Protokolldefinition laden möchte. |
Problematisch ist aktuell, dass ich die Hashes die für die Tests geladen werden auch in einen Package mit gleichem Namen gepackt habe. Das Überschreiben der Methode zum abholen der Daten klappt sogar auf diesem Weg und für den Test bekommt man dann die Daten aus dem Package was als 2. geladen wurde. Was nicht klappt ist dann wieder das originale Package laden. Das ist ja bereits geladen und der Weg zurück klappt nicht. In dem Package könnte man vermutlich noch mit Versionen arbeiten: http://blogs.perl.org/users/grinnz/2018/04/a-guide-to-versions-in-perl.html Allerdings weiss ich nicht, ob wir über das Fhem Update überhaupt unterschiedliche Versionen bereitstellen können. |
Mir ist aufgefallen, daß Du bei filterfunc
|
Vermutlich habe ich noch structure refs verwendet, da Perl der Meinung war es wäre keine Referenz. Ist mir damals aber schon nicht klar gewesen, wieso method nicht auf eine Referenz verweist, da doch auf eine sub Referenziert wird. |
Bei filterfunc steht in method ein string |
Damit das mit signalduino_protocols.pm und Package funktioniert müssen die Referenzen bei postDemodulation und method auch in strings umgewandelt werden. |
Bist Du der Meinung die Anpassung aus diesem PR funktioniert nicht? |
wenn ich es so wie in 88_HMCCU.pm und HMCCUConf.pm mache, dann funktioniert es als |
Mit dem Auslagern in ein Package habe ich mich generell beschäftigt und nicht so sehr auf andere FHEM Beispiele geschaut. Hast Du dir die Änderungen in diesem PR angesehen? |
arbeitest Du in diesem PR mit einer Referenz oder einer Kopie vom Protokollhash? |
Die korrekte Antwort lautet wohl beides. Ich habe den Rückgabewert der sub SIGNALduino_LoadProtocolHash beibehalten. Meiner Meinung auch der Zweck der Kapselung. Wo die Daten her kommen ist egal, solange das bereitstellen für das Modul sich nicht ändert. Aus dem Package wird eine Referenz geladen, aber diese Referenz wird dereferenziert und dann von der Sub zurückgeliefert. Ist also identisch zu dem bisherigen Verhalten. |
Durch das beibehalten der sub SIGNALduino_LoadProtocolHash ersetzt Du meiner Meinung nach eine unkonventionelle Lösung durch eine andere unkonventionelle. Für die Tests sehe ich bei einer Referenz auf den Hash keine größere unlösbare Probleme, es fällt sogar ein Test (Load Protocolhash) weg.
Die Defaults können auch bei einer Referenz vom Hash gesetzt werden. Ich denke das Verändern von Daten eines anderen Packages ist nur ein kleine unsauberkeit, da der Hash ja zur 00_signalduino gehört. |
Als Referenz zum Hash sieht es so aus:
und in der 00_Signalduino:
|
42aa950
to
d54a49f
Compare
Was ist daran unkonventionell? FHEM lädt Module auch über eine eigene Sub nach um z.B. auf Fehler reagieren zu können.
Welche?
In das Modul kann man alles mögliche packen, ja das geht. Die sub
Perl hat für die Angabe der Version bereits einen Standard. Wieso sollten wir den nicht einfach verwenden?
Das verstehe ich nicht. Der Test soll feststellen, ob das Laden der Protokolldaten funktioniert.
Können kann man es aber ich würde das nicht machen wollen wenn es nicht notwendig ist. |
Mal abgesehen davon, dass ich das Package SD_Protocols benannt habe sehe ich das jetzt keinen Unterschied. Das Package gibt eine Referenz zu dem Hash zurück. Ich habe es über eine sub realisiert und bin nicht direkt auf die Variable im Package gegangen. Es ist allgemein eine good practice, auf Daten andere Module nicht direkt zuzugreifen, sondern das über Routinen zu machen. Line 287 in 39e1aae
RFFHEM/FHEM/lib/signalduino_protocols.pm Lines 2211 to 2213 in 39e1aae
|
39e1aae
to
666cc78
Compare
Pull Request Test Coverage Report for Build 1219
💛 - Coveralls |
Ich hab den PR mal so weit fertig, dass
Wie allerdings die Protokolldaten unabhängig vom Modul aktualisiert werden könnten ist offen, da ja alles in einem Repository liegt. Aus #469 sind wir also damit bei der dort beschriebenen Variante 1.
|
@HomeAutoUser @elektron-bbs |
Ich habe die Anpassung in dev-r34 überführt. @Ralf9 Du warst drauf und dran mit einer Referenz und nicht mit einer Kopie zu arbeiten. |
Wenn ich das richtig verstehe, ist das so wie ich es bei mir gemacht habe (s.u) das gleiche, als wäre der hash in der 00_SIGNALduino.pm signalduino_protocols.pm
00_SIGNALduino.pm
|
Für mich scheint das, so wie Du es gemacht hast, nicht so richtig zukunftfähig zu sein. Bei der bewährten Lösung, so wie es Rudi und betateilchen es vorgeschlagen haben, ist es recht einfach möglich weitere Hashes in die signalduino_protocols.pm hinzuzufügen. Die Version bei Deiner signalduino_protocols.pm ist mir zu einfach. Es ist auch zu überlegen ob wir die hash-Datei bei der Entwicklungsversion |
Hab wegen der evtl umbenennung auf |
Was ist dabei die Sonderlösung? Kannst Du das genauer definieren? Ich habe da einige Standard Perl Varianten recherchiert und nach best practices gesucht. Das Einfügen von weiteren Variablen ist hier auch nicht schwierig, oder wo siehst Du da Probleme? Was die Bezeichnung der Version angeht, habe ich mich einfach an einen wohl definierten Standard gehalten: Wenn das ganze Perl Universum damit arbeiten kann, wieso soll das bei uns nicht auch gehen? |
Sonderlösung ist, daß ich dies außer bei Dir noch nirgends gesehen habe. In fhem und in den Beispielen wo ich im Internet gefunden habe, wird es so gemacht wie es Rudi und Betateilchen empfehlen. Wenn ich das richtig überblicke brauchst Du für jeden Hash eine eigene Methode um über die Loadroutine eine Kopie zu laden. |
Das kommt eher aus der Richtung der Objekt orientierten Programmierung.
Brauchen nicht, aber sauberer ist es, um an die Daten zu kommen, vor allem wenn wir das Modul eher als Objekt sehen und nicht nur als Text Datei.
Das mache ich, weil der Inhalt des Hashes im SIGNALduino Modul verändert wird. Das wäre unsauber und schwierig wartbar, wenn dann z.B. das SD_Tool auch noch auf den Hash Zugreift und daran was verändert. Am Ende ändert man besser keine Daten anderer Packages. Offenbar kenne ich die Variante auch nicht, welche dir vorschwebt.
Optimal ist Abhängig von den Anforderungen. Dass wir die Daten auf dauer nicht kopieren müssen wäre durchaus denkbar, wenn wir in SD_Protocols ein paar Schnittstellen einbauen wie z.B.
Verrätst Du mir bitte noch deine ohne Kopie Methode. |
Danke für den Hinweis, das war mir nicht bewusst, daß damit auch eine Kopie angelegt wird, demnach arbeite ich mit meiner Methode auch mit einer kopie. Ich habe wegen den hash Referenzen nochmals im Internet gesucht und nachgelesen, nun ist es mir etwas klarer geworden. Wollte ich ohne eine Kopie arbeiten, müsste ich es so machen: |
Bei der Gelegenheit könnten wir auch dies bereinigen:
Bei postDemodulation wird eine coderef verwendet und bei filterfunc ein String.
Außerdem ist mir noch aufgefallen, daß nicht alle postDemodulation Subroutinen im Namen ein |
Ja, so könnte man es machen. |
Die Referenz auf eine Sub ist der String Variable vorzuziehen, da im Falle der Referenz direkt auf die Speicheradresse verwiesen wird. |
Der String lässt sich auch noch nachträglich in eine Codereferenz wandeln: |
Ja, aber wozu einen String im Speicher ablegen, wenn wir ohnehin am Ende eine Speicheradresse benötigen? Dann können wir doch gleich die Speicheradresse dort hinterlegen. |
Damit der Name der Subroutine im log ausgegeben werden kann. |
Das war Unwissenheit und ist eine Altlast. Der Name kann auch über die Coderef herausgefunden werden, wenn der wirklich wichtig ist. |
Ich habe es mit svref_2object versucht, es ist umständlicher und aufwändiger als wenn der String im Speicher abgelegt wird. |
Das B Modul ist ein Core Modul. Wieso packen wir den Code nicht einfach in eine Sub? |
Würde es nicht auch reichen den Namen innerhalb der Sub zu kennen? |
Wenn der Name der Subroutine im Speicher abgelegt wird, dann sieht die log Ausgabe (verbose 5) ungefähr so aus:
Wenn wie seither die Codereferenz abgelegt wird, dann sieht die log Ausgabe ungefähr so aus.
Ich würde es sauberer finden wenn alle postDemodulation Subroutinen "postDemo" im Namen enthalten würden. Damit alles gleich ist sollte dann auch bei der filterfunc die codereferenz abgelegt werden. Der Name der filterfunc Subroutine kann in einer log Ausgabe am Ende der Subroutine ausgegeben werden. |
Mein Vorschlag wäre dann:
|
Ich bin gerade dabei das ganze bei mir einzubauen. |
Durch {SIGNALduino_LoadProtocolHash("$attr{global}{modpath}/FHEM/lib/signalduino_protocols.pm");} Wird der Protocol Hash neu geladen |
Protocol data ist moved to a fully compatible perl module and imported via standard perl operations.
The protocol data ist stored in an text file and loaded at runtime into a hash variable.
Th file ending itself is not compatible with standard fhem Update.
Because the new approach uses a perl module for storing the data, it uses a file type that is supported bybstandard fhem Update procedure.
Currently there ist no known breaking Change.