Closed Morg42 closed 5 months ago
onkelandy
commented
5 months ago Groß reviewen kann ich da nicht, maximal testen. Generell bin ich auch für Variante b. Ich glaube nicht, dass irgendjemand den struct-Namen, der zB ja bei Plugins nicht mal selbst definiert werden kann, fürs Item braucht. Zumal, was passiert beim Einbinden von mehreren structs? 1st wins? Macht ja auch wenig Sinn...
msinn
commented
5 months ago Ich verwende den Namen aus structs.Für mich wäre c die passende Variante.
Den Punkt
finde ich nicht entscheidend. Namensdubletten kann man beliebig erzeugen, z.B. eifach durch mehrfache Vergabe des gleichen Namens bei Items. Der Name ist zur eindeutigen Adressierung von Items daher sowieso nicht nutzbar.
Das ist ein seltener auftretender Fall, als bei Variante b.
Morg42
commented
5 months ago @msinn Hättest du einen Vorschlag, wie der Schalter/die Option implementiert werden kann? Dann baue ich das ein 🤗
Morg42
commented
5 months ago Unter der Voraussetzung, dass wir den fix a) implementieren wollen und b) schaltbar machen:
Ich habe mal lib/smarthome.py und lib/config.py durchgesehen -
ich könnte problemlos in etc/smarthome.yaml einen Parameter (zB) struct_strip_name: <bool> einsetzen, der wird ja automatisch in der sh-Instanz als Attribut angelegt.
Durch den prozeduralen Aufruf der lib.config habe ich die sh-Instanz nicht zur Verfügung; eine lib.smarthome.get_instance()-Methode gibt es (anders als bei zB Items) nicht, und das sh-Objekt wird nicht durchgereicht.
Ich sehe als Möglichkeiten
Ich würde bei der Abwägung zwischen Aufwand und Ergebnis die Variante wählen, lib/smarthome.py mit einer Instanzverwaltung auszustatten. Das sollte den normalen Betrieb nicht beeinträchtigen (sh wird eh nur einmal und nur an einer Stelle instanziiert), ermöglicht von "überall" über my_sh = Smarthome.get_instance() den Zugriff auf die aktuelle sh-Instanz und ermöglicht damit einfach den Zugriff auf die "Systemvariablen" aus smarthome.yaml.
Ich sehe mögliche Probleme in der Testumgebung, falls doch mehrere sh-Instanzen erzeugt werden sollten. Das würde ich testen und ggf. einen entsprechenden Workaround einbauen (zB die Instanz nicht zu veröffentlichen, wenn man in der Testumgeung läuft)
Bevor ich mich daran setze - gibt es Meinungen dazu, Argumente (insbesondere) dagegen? Aspekte, dir mir entgangen sind?
Morg42
commented
5 months ago Der neue Parameter heißt 'struct_strip_name', ist per default False und kann in der smarthome.yaml gesetzt werden.
default ist old behaviour, also non-breaking. Wer es "neu" haben möchte, muss den Parameter per Hand hinzufügen.
Das SmartHome-Objekt hat jetzt eine neue Methode get_instance(), die die Instanz zurückgibt. Eine zweite Instanz kann instanziiert werden, erzeugt eine critical-Logmeldung und hat sonst keine Auswirkungen, von denen ich weiß ;)
Teste laufen auch mit Instanzprüfung problemlos.
Wenn wir uns entscheiden wollen, dass wir das so machen, schreibe ich vor dem Merge noch die Doku dazu.
bmxp
commented
5 months ago Ich kann hier leider nix beitragen weil ich structs bisher nur sehr selten genutzt habe
onkelandy
commented
5 months ago Tested the new paramater (and without). Works as expected when using global structs. Also works with plugin structs and "individual" structs.
Vorschlag zur Lösung für fix #594
Vor dem Mergen des Structs ins Item-Template wird ein ggf. vorhandenes Attribut 'name' entfernt, das Item behält seinen ursprünglichen Namen.
TODO: Aus meiner Sicht gibt es drei Wege: a) der Name aus dem Struct überschreibt den des Items (status quo) b) der Name des Structs wird ignoriert (PR) c) der Name des Structs wird nur übernommen, wenn das Item keinen Namen hat.
Vor- und Nachteile: a) + nonbreaking für alle möglichen Anwendungen
Mein Favorit wäre auch b), weil ich die Übernahme des Namens struct -> item unlogisch finde.
Man könnte einen globalen Parameter (smarthome.yaml?) einführen, der im default den neuen Modus verwendet und bei ausdrücklicher Nutzung das alte Verhalten beibehält -- ich halte das allerdings für relativ unwahrscheinlich.
Vorschläge, Meinungen?