Skip to content
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

Enable more manufacturers and devices #27

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

christian-weiss
Copy link

Durch doppelte manufacturer-keys waren die Hersteller "GL-Inet" und "WD My Net" nicht mehr im Firmware Download Assistenten verfügbar (https://firmware.freifunk-muensterland.de/md-fw-dl/).
Der Bugfix behebt das. Damit sind 4 weitere Geräte nun wieder im Assistenten verfügbar.

Zudem werden die keys nun wieder aufsteigend durchnummeriert und das JavaScript Statement sauber mit Selekolon terminiert.

"8gl-innovations": {id: "gl", name: "GL Innovations"},
"9linksys": {id: "linksys", name: "Linksys"},
"10netgear": {id: "netgear", name: "Netgear"},
"11alfa": {id: "alfa-network", name: "ALFA-Network"},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wollen wir nicht einfach nur auf Zahlen umstellen? Diese Nummerierung+Name ist irgendwie komisch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dafür. Vermutlich wurde das gemacht weil es so einfacher ist eine Zeile zwischen zu fügen ohne die nachfolgenden aufrutschen zu müssen.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nur wenn man nummeriert, dann sollte man es auch durchhalten, daher der PR.

Ich finde die Nummerierung grundsätzlich nicht schön, weil sie keinen Mehrwert liefert.
Wenn man neue Werte einfügen möchte der die Reihenfolge der Einträge verändern will, dann ändern muss man, um die aufsteigende Nummerierung durchzuhalten immer x Zeilen aktualisieren. Das erzeugt im Review nur unnötiges Rauschen.

Eigentlich ist es ja nur wichtig das jeder Key eindeutig ist - hierzu würde ich gerne schlicht den Wert aus "id" für den key wiederverwenden/spiegeln. Das würde ich machen, damit der Testaufwand gering ist.

Wenn mal einer Lust das JavaScript-Drumherum anzupassen, dann könnte man das Object aus zu einem Array of Object machen (ohne keys; nur noch Elemente, die aus id+name Objekten bestehen). Das mache ich aber nicht.

Viel wichtiger finde ich aber die User Experience und da wäre es schön, wenn man gemäß "name" alphabetisch sortiert - entweder im Script oder direkt hier in der config. Letztes würde ich machen - damit der Test-Aufwand gering ist.

Was meint Ihr?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Es gibt quasi nur zwei Marken, nah gut inzwischen drei, die verwendet werden: TP-Link, Ubiquiti und AVM. Die sollen oben stehen. Daher haben wir es nicht alphabetisch sortiert sondern diese komischen Zahlen davor gesetzt.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, wenn es um die Sortierung geht, dann wäre es gut, wenn das JavaScript kein "sort by key" macht und einfach die Reihenfolge nimmt, wie sie in der Config vorkommt.

Wenn man das JavaScript aber nicht anpassen möchte (ich finde die Code Stelle gerade nicht), dann wäre ich dafür das zumindest im Keynamen (semantisch) kenntlich zu machen - z.B. position01, position02, etc. bzw. mehr im Streuspeicherverfahren: position0100, position0200, damit man nachträglich Einträge (mittig) ergänzen kann, z.B. position0150, noch später dann position0125 oder pos0175, um nicht bei jeder Einfügung gleich "alle" anderen keys neu durchnummerieren zu müssen.

Copy link
Author

@christian-weiss christian-weiss Oct 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

position: netter Nebeneffekt: führende Nullen würden ansonsten ggf. abgeschnitten
Ich erstelle dafür einen neuen Commit.

config.js Show resolved Hide resolved
.idea/
/*.iml
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Das ist IDE-spezifisch. Wollen wir sowas aufnehmen oder sollen die Leute lieber eine persönliche .gitignore pflegen?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich glaube es ist einfacher die persönlich zu pflegen. Da unter Umständen für mehrere IDEs Einträge zu Pflegen fände ich irgendwie unschön.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist eine Frage der Kosten-Nutzen-Balance.

Vorteil:
a) "Schutz" des Repos vor unnötigem Kram (hilft allen Teammitgliedern)
b) hilft dem Committer ("privates" lokal zu halten; "Dusseligkeitsschutz")

Nachteil:
a) Maximal genauso viele IDE-spezifische Einträge wie es Teammitglieder gibt; mit etwas Glück gibt es Synergien

"Frisst nicht viel Brot" (Mehr Nutzen als Nachteil), daher stimme ich dafür.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Egal wie wir uns entscheiden. Wir sollten es über alle Repos konsistent machen.
Siehe: https://github.com/FreiFunkMuenster/site-ffmsl/pull/2/files#diff-a084b794bc0759e7a6b77810e01874f2R3-R4

Ich stimme für (a). Was meint Ihr @HellMar @MPW1412 ?
Diese Conversation würde ich gerne auf resolved setzen und den PR abschließen.

Comment on lines -100 to -108
"8alfa": {id: "alfa-network", name: "ALFA-Network"},
"98devices": {id: "8devices", name: "8devices"},
"10meraki": {id: "meraki", name: "Meraki"},
"10nexx": {id: "nexx", name: "Nexx"},
"11openmesh": {id: "openmesh", name: "OpenMesh"},
"12onion": {id: "onion", name: "Onion"},
"13wd-ny-net": {id: "wd-ny-net", name: "WD My Net"},
"13wd-ny-net": {id: "raspberrypi", name: "Raspberry Pi"},
"14x86":{id:"x86",name:"x86"}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum wurden diese Klassen entfernt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicht entfernt, nur neu durchnummeriert. (meine Meinung zu diesen Nummern: siehe oben)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ne, es wurden 17 Zeilen entfernt und nur neun hinzugefügt. Folglich fehlen welche...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17 vorher, 17 nachher = 0 entfernt (14 umbenannt)

.idea/
/*.iml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich glaube es ist einfacher die persönlich zu pflegen. Da unter Umständen für mehrere IDEs Einträge zu Pflegen fände ich irgendwie unschön.

config.js Show resolved Hide resolved
"8gl-innovations": {id: "gl", name: "GL Innovations"},
"9linksys": {id: "linksys", name: "Linksys"},
"10netgear": {id: "netgear", name: "Netgear"},
"11alfa": {id: "alfa-network", name: "ALFA-Network"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dafür. Vermutlich wurde das gemacht weil es so einfacher ist eine Zeile zwischen zu fügen ohne die nachfolgenden aufrutschen zu müssen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants