search

ffdo / node-informant

Small utility to collect node information in a Freifunk network via announced
7 stars 4 forks source link

Refactoring #17

Closed corny closed 7 years ago

corny commented 8 years ago

Der Branch kann noch nicht gemerged werden.

Ich habe mir den Code genauer angeschaut und mit einem Refactoring der ProcessPipe begonnen. Weil die Process-Methode in keinem Fall die Eingabe verändert, ist die unveränderte Ausgabe der Eingabe (über Pipes) nicht sinnvoll. Die Verwendung von unbuffered Pipes verhindert momentan Nebenläufigkeit, anstatt sie zu unterstützen. Um das umbauen zu können, muss ich kritische Sektionen finden und exklusive Zugriffe mit z.B. Locks sicherstellen.

Irgendwelche Einwände?

dereulenspiegel commented 8 years ago

Die Idee ist gut. Aber teile bitte den PR auf. Einen für die Änderung der ProcessPipes, einen für Arbeiten an der HTTP-API, und ggf. weitere für andere Themenfelder. Über die verstärkte Verwendung von anonymen Variablen muss ich nochmal nachdenken. In meinem aktuellen Flow, macht es das Debuggen eventuell schwieriger. Aber vielleicht gibt es dlv auch eine Möglichkeit das besser zu handeln.

dereulenspiegel commented 8 years ago

Ein Hinweis noch. Ich refactore gerade das Repo damit es mit Standard go vendoring kompatibel wird. Es könnte also sein, dass du rebasen musst o.ä.

corny commented 8 years ago

Ok, danke. Die ersten vier commits könntest du schon mal mergen/cherry-picken.

dereulenspiegel commented 8 years ago

Ich habe das repo bereits von gb auf die Standard-Tools migriert. Du müsstest das ganze also erstmal auf den aktuellen Stand rebase, da sich die Verzeichnisstruktur geändert hat. Langfristig macht das das Handling aber einfacher.

corny commented 8 years ago

Bitte #18 mergen.

corny commented 8 years ago

Aus meiner Sicht kann das hier jetzt gemerged werden. Die nächste auf meiner Liste stehenden Änderungen sind umfangreicher.

corny commented 8 years ago

Eine Frage noch: Sollen neben dem announced-Receiver wirklich noch weitere integriert werden?