visu Plugin: Fixed 'False' bug discovered by psilo #25
Conversation
- extended README.md of hue plugin
Develop update from smarthomeNG:Develop
…develop * 'develop' of https://github.com:/msinn/smarthomeNG: (22 commits) removed some debug messages from plugin add Pull request 18, 19 and some system values in env. items - enabled smarttv plugin for new logging system - enabled wol plugin for new logging system multiknx merge from encbladexp every module in lib has now its own logger logging.yaml wird wieder ignoriert bugfix for http listener updated DWD and tankerkoenig plugin to new logging system enabled new logging system for eta_pu and avm plugin - tankerkoenig-plugin: further text correction in readme more info in readme - small bugfix for keys - readme updated logging refactored, add etc/logging.yaml file for configuration systemair modbus plugin v0.1 - special bugfix reported by beta tester - updated version to 0.914 ultra comfort function: initializes callmonitor items in case they are empty (from get_calllist function) added calendar class type (e.g. private) decoding to ical plugin; further enocean documentation Layout fixes to readme small modifications to documentation ...
|
Kennst Du aus smarthome.py die Funktion string2bool() ? |
|
fände ich gut, würde mein avm plugin dann auch anpassen |
|
Ja, ich war vor einiger Zeit als ich ein Plugin für mich schrieb darüber gestolpert. Da die Funktionen jedoch nicht dokumentiert waren war ich am zweifeln, ob die Funktionen als allgemeines API für Plugins zur Verfügung stehen, oder nur für den smarthome core und evtl. ohne Ankündigung modifiziert werden. In diesem Programmers Guide könnten auch Leitlinien zur Definition und Validierung von Properties niedergeschrieben werden. Ich bin z.B. nicht glücklich mit der Art wie im originalen visu Plugin der Parameter smartvisu_dir implementiert wurde. So wie der Parameter genutzt wird, hat er zwei unterschiedliche Datentypen:
Dabei würde eine Standard-Validierung nicht helfen, weil unter anderem der Boolean Wert True kein sinnvoller/gültiger Wert für diesen Parameter ist. Wenn wir entsprechende Guidelines erstellen, würde ich das Plugin entsprechend umbauen, wobei wir sehen müssen, wie wir dabei mit der Rückwärtskompatibilität umgehen. |
|
Ich fände es gut, dabei auch gleich noch eine Funktion string2int() zu ergänzen. Häufig wird einfach in der Form self.log_telegrams = int(log_telegrams) gewandelt. Wenn dann jemand in der conf einen nichtnumerischen Wert einträgt, schmeisst das plugin gleich eine exception. |
|
👍 fuer Vereinheitlichung. Veilleicht kann man diese Sachen noch weiter vereinheitlichen, so dass es nur eine Stelle gibt, an der Typen umgewandelt werden. Dabei habe ich auch noch die Sachen in Abgesehen davon, wuerde ich auch gerne eine Basis-Klasse fuer Plugins haben, die z.B. auch solche Sachen wrappen koennte. Der eigentliche Grund dafuer ist, dass Plugins ziemlich flexibel sind und nicht standardisiert. Es gibt eine zentrale Stelle, mit der man z.B. den Start-Prozess fuer ein Plugin implementiert oder den Stopp-Prozess. Teilweise werden auch Scheduler registriert, die z.B. bei mehrfacher Verwendung eines Plugins Probleme machen, da der Name dann nicht mehr unique ist (oder das Plugin behandelt das explizit - waere aber was, was vereinheitlicht werden sollte; siehe z.B. auch PR #81, wo das schon z.B. fuer das SML-Plugin implementiert ist). |
|
Noch was zu:
Ich faende es gut, wenn das einen Fehler produziert, da man nur so erkennen kann, dass die Konfiguration falsch ist. Wenn ein numerischer Wert konfigurierbar ist, sollte das auch ein numerischer Wert sein (oder etwas, was zu einem numerischen Wert konvertiert werden kann). |
|
Ich habe auch nicht gefordert keinen Fehler zu produzieren. Ein sauberer Log Eintrag ist hier der richtige Weg. Eine Exception zu schmeissen halte ich jedoch für verheerend. |
|
Was ist denn nun? Ich habe keine Idee was ich tun soll. Es gibt keine Richtlinien um Parameter Validierung in Plugins zu machen nach der ich das Plugin umstellen kann. Andererseits hängt der Bugfix wegen der von mir gewählten Art der Validierung nun seit 4 Tagen und scheint vor sich hin zu gammeln. Er wird weder gemerged noch geclosed. |
|
ich hätte auch gerne eine lsg... würde ggf auf die string2bool umsteigen. in den configparametern sollte imho dann aber nur True und False erlaubt sein. |
|
ps: ich könnte auf merge drücken, hätte aber gerne eine von allen akzeptierte lsg... |
|
Ich baue es um, nachdem wir eine allgemeine Lösung haben. Ich finde es im Moment nur etwas irritierend, dass ein Parameter Handling, dass auch in anderen Plugins aktuell genutzt wird, und dass ich im Visu Plugin für andere Parameter bereits vor diesem PR genutzt habe, nun verhindert, dass ein Bugfix eingespielt wird. Wir sollten die Grundsatz Diskussion (und Lösung) ein wenig vom "Tagesgeschäft" trennen, sonnst kann bald kein PR mehr umgesetzt werden. |
|
muss gestehen ich habe den pr bisher weil ich selber am coden war nicht angesehen. bin aber auch nicht alleine hier ;-) schaus morgen frueh gleich an. bin aktuell leider nur am mobilgeraet online. keine boese absicht oder so. schlicht zeitmangel |
|
War keine Absicht mit dem PR, dachte der ist schon merged. |
|
Danke! :-) Der nächste PR (Weiterentwicklung) kommt morgen. |
visu Plugin: Fixed 'False' bug discovered by psilo
No description provided.