PR-Review - Ich brauche euch!

Entwicklung
1

Hallo zusammen,

Ich denke, es wäre gut, wenn wir ein kleines Thema hätten, in dem wir uns gegenseitig für PR-Reviews taggen können, also hier ist es :slight_smile:

Es gibt immer mehr PRs, und ich glaube nicht, dass ich alleine alles reviewen kann. Damit keine PRs unbeantwortet bleiben, wäre es cool, wenn wir uns gegenseitig bei den Reviews helfen könnten!

Aktuell gibt es einige PRs, die reviewed werden müssen.

Ich denke dabei insbesondere an:

Wie reviewt man eine PR?

Code-Review

Den Code zu reviewen ist einfach: Ich lese den gesamten Code und sehe, ob mich etwas stört, in Bezug auf die reine Codequalität. Variablenbenennung usw…

Im Grunde genommen lest ihr den Code so, als wärt ihr Entwickler der PR, und fragt euch, ob ihr es so gemacht hättet. Wenn euch etwas stört, hinterlasst Kommentare, um weitere Informationen anzufordern, und wenn es wirklich nicht in Ordnung ist, hinterlasst einen Kommentar und markiert die Review als „Änderungen angefordert“.

Vermeidet es, in den Kommentaren aggressiv oder abwertend zu sein, das Ziel ist positiv zu sein: Wir wollen nur die PR voranbringen und eine maximale Codequalität erreichen :slight_smile:

Funktionale Review

Wenn ich eine PR reviewen, hole ich den Branch der PR mit den Befehlen, die von GitHub bereitgestellt werden:

Screenshot 2021-01-18 at 17.29.13
Screenshot 2021-01-18 at 17.29.131890×868 99.8 KB

Dann starte ich den Server:

cd server
npm install
npm start

Dann das Frontend:

cd front
npm install
npm start

Dann teste ich absolut alles: Ich versuche, die Integration zu brechen, indem ich alles mache, was ein Benutzer tun würde: eintreten, austreten, wieder eintreten, die gleiche Seite mehrmals öffnen, um sicherzustellen, dass nichts kaputt geht.

Nach dem Lesen des Codes weiß ich in der Regel ein bisschen, was kaputt gehen wird: Zum Beispiel haben viele Integrationen Seiten zum „Scannen“ von Geräten. Ich möchte sicherstellen, dass diese Seiten nicht bei jedem Laden der Seite einen Scan starten, sonst wenn der Benutzer 10 Mal auf die Seite geht und es 10 parallele Netzwerk-Scans gibt, ist das zu viel ^^ Wenn es eine Scan-Seite gibt, öffne ich in meinen Tests 10 Mal die Scan-Seite in 10 verschiedenen Tabs und sehe, was passiert :smiley: (Und oft geht es dann kaputt aha)

Vielen Dank an alle, die mir helfen, PRs zu reviewen, es ist genauso positiv, PRs zu reviewen wie PRs zu erstellen :pray:

@damalgos @VonOx @cicoub13 @Terdious @AlexTrovato @Pti_Nico @Albenss @bertrandda @Hotfix31 @Reno @Lokkye @simon_raynaud @billona @Exilon62 @ProtZ @euguuu

3

Hop, ein neues PR, das Feedback braucht, die Netatmo-PR von @damalgos und @Terdious!

Hier testen:

4

Ich teste sie gerade. Wir besprechen das am Samstag mit @Terdious.

5

Nach einer ersten Review mit @Tlse-vins stellte sich heraus, dass einige Bedingungen für die for each-Schleifen nach Anfragen nicht getestet wurden. Wir haben das Problem gelöst, also muss ich einen neuen Push durchführen. Wie läuft das ab, wenn die PR in der Review ist? Muss ich die Review-Anfrage erneut stellen? Oder ist nichts weiter zu tun?

6

Man muss die Änderungsanfragen auf GitHub als behoben markieren

7

Ok !! Vielen Dank für die Antwort @VonOx.

8

Re !!

Könnte mir jemand helfen… ich habe einen eslint-Fehler… normal… aber ich weiß nicht, wie ich ihn beheben soll. Also werdet ihr mir sicher sagen, dass das eine Noob-Frage ist. Aber ich muss wohl nicht gut im Suchen sein, denn ich finde keine Antwort. Ich habe eine Funktion erstellt, um den Hinweis von @AlexTrovato auf die Wiederholung in der Netatmo-PR zu korrigieren. Nun kann einer der Eingabeparameter (newValue) eine {number} oder eine {boolean} sein. Daher habe ich nichts in @param angegeben. Es funktioniert gut, aber eslint gibt mir einen Fehler. Gibt es eine Möglichkeit, eslint dafür zu deaktivieren oder mehrere Datentypen für einen Parameter anzugeben? Ich habe mehrere Dinge versucht, aber nichts hat funktioniert.
Vielen Dank im Voraus

Edit: Es ist gut, ich habe es gefunden ^^ Ich habe nicht weit genug geschoben ^^ Für den Fall, dass andere Personen suchen, lasse ich meinen Beitrag und den Link zur Dokumentation zu diesem Punkt hier: https://eslint.org/docs/2.0.0/rules/valid-jsdoc

9

Hallo,

für Fälle mit mehreren Typen für Parameter oder Rückgabewerte muss man | verwenden

@param {(string|string[])} [somebody=John Doe] - Der Name einer Person oder ein Array von Namen.
10

Vielen Dank @wazalop!! Ich habe gerade die Antwort gefunden und meinen Beitrag bearbeitet!! Trotzdem ein großes Dankeschön für deine Rückmeldung!!

11

Und zwei neue PRs zum Reviewen!

Ich freue mich über jede Hilfe beim Testen dieser beiden! :slight_smile:

12

Hallo !!

Nach dem Test des PR

Ich habe die vollständigen Tests dieser letzten Version auf FR-Seite durchgeführt und einige EN entfernt, indem ich einige etwas buggy Sachen versucht habe (etwas hastig geschrieben) und solange es sinnvoll bleibt, funktioniert es gut für mich. Die Anzeige der Tage statt der Uhrzeit ist gut. Dennoch werden die Tage auf Englisch angezeigt, obwohl die Oberfläche FR ist. Ich empfehle daher die folgenden Änderungen, um die FR-Sprache zu berücksichtigen:

  • In der Datei front/src/config/i18n/fr.json - Zeile 17 nach "save": "Enregistrer" - hinzufügen von:
    "save": "Enregistrer",
    "daysOfTheWeek": {
      "monday": "Lundi",
      "tuesday": "Mardi",
      "wednesday": "Mercredi",
      "thursday": "Jeudi",
      "friday": "Vendredi",
      "saturday": "Samedi",
      "sunday": "Dimanche"
    }
  • In der Datei front/src/config/i18n/en.json - Zeile 17 nach "save": "Save" - hinzufügen von:
    "save": "Save",
    "daysOfTheWeek": {
      "monday": "Monday",
      "tuesday": "Tuesday",
      "wednesday": "Wednesday",
      "thursday": "Thursday",
      "friday": "Friday",
      "saturday": "Saturday",
      "sunday": "Sunday"
    }

In der Datei front/src/components/boxs/weather/WeatherBox.jsx - Zeile 270 - ersetzen:

<div className="col-5">{day.datetime_beautiful}</div>

durch:

<div className="col-5">{<Text id={`global.daysOfTheWeek.${day.datetime_beautiful.toLowerCase()}`} />}</div>
Funktioniert bei mir:

image
image372×410 12.1 KB

Schließlich diese PR nutzen, um den kleinen Bug der Anzeige der Windeinheit zu korrigieren, die in metrischen Einheiten Meilen/Stunde anstatt km/h anzeigt:
image. Ersetzen Sie in der Datei front/src/components/boxs/weather/WeatherBox.jsx ebenfalls - Zeile 179:

{props.units === 'si' ? 'km/h' : 'm/h'}

durch (ein kleines console.log zeigt uns, dass props.units entweder ‹ metric › oder ‹  › zurückgibt) indem wir einen Leerraum zu den Einheiten hinzufügen, damit es schöner aussieht:

{props.units === 'metric' ? ' km/h' : ' m/h'}

Funktioniert bei mir: image

In der Hoffnung, dass ich hilfreich war. Ich gehe zum nächsten PR
Mit freundlichen Grüßen, Thomas.

13

Die Szenen wurden zum Testen erstellt. Jetzt müssen wir nur noch warten.
Allerdings gibt es bereits einen Hinweis. Nirgendwo wird angegeben, was vorab konfiguriert werden muss, um diese Szene nutzen zu können. Vielleicht wäre eine Warnmeldung in der Trigger-Box sinnvoll, falls die Koordinaten des Hauses (ich nehme an, das ist die einzige Voraussetzung) nicht angegeben sind — zumal dort Platz ist:

image
image1180×389 13.7 KB

@pierre-gilles:
Beim Testen eines langen Hausnamens habe ich einen „Bug“ entdeckt: Wenn man einen langen Namen eingibt, z. B. ‹ Maison Test Sunrise-Sunset sans Emplacement ›, erhalten wir beim Speichern eine nicht passende Fehlermeldung:
image
image884×742 62.8 KB

statt „Validierungsfehler: Der Name Ihres Hauses ist zu lang“.
Problem nur auf FR.
Issue #1055 erstellt. Ich werde versuchen, den PR so schnell wie möglich zu erstellen.

Edit: Der PR ist fertig und zur Überprüfung verfügbar … ich denke ^^ : PR #1056

14

Hallo @Terdious! Danke für deine Reviews, versuche beim nächsten Mal, die Review direkt auf Github zu machen, dafür ist es da und es ist einfacher, als hin und her mit dem Forum zu gehen :slight_smile:

Bei der PR zum Wetter glaube ich, dass du nur die Spitze des Eisbergs getestet hast, der größte Teil der PR war der Teil „Gespräch mit Gladys“, es gibt Dutzende neuer Sätze, die hinzugefügt wurden, und man muss sie einen nach dem anderen testen und überprüfen, ob sie alle funktionieren. Bei meinem letzten Test war das nicht der Fall, aber normalerweise gab es seitdem Verbesserungen!

Bei der PR zur Sonne, gute Anmerkungen. Auch hier ist es besser, wenn die Anmerkungen direkt in der PR gemacht werden :slight_smile:

15

Re,

Also

Dafür ist das Problem, dass ich die von mir angeforderte Änderung in der Seite der Commit-Änderungen nicht sehe. Daher konnte ich die betreffenden Dateien nicht direkt auf Github überprüfen. Aber ich habe den Link zur Gladys-Community hinterlassen

image
image928×280 17.9 KB
, da ich dachte, dass aufgrund der Länge des Beitrags dies etwas unordentlich im Github-Thread wäre. Dennoch, da ich die Github-Review-Praktiken noch nicht gut kenne, werde ich es beim nächsten Mal so machen, wenn es bevorzugt wird :slight_smile:

Für den Testteil habe ich mich auf Folgendes gestützt:

image
image1657×673 27.3 KB

Plus toute la partie réponse. Alors en effet, je n’ai pas pu tout tester car je n’ai pas de neige prévu. Mais pour le reste tout était fonctionnel pour tout les jours de la semaine, demain et après-demain ; et pour tout les types de questions j’ai fais les tesqts avec et sans accent, avec des espaces supplémentaires entre 2, avec des petites fautes de frappes (ex. : « Donn moi la meteo » ou « Donne le meteo ») et tout les retours étaient ceux attendu. Pour la partie que je n’ai pas pu testée (la neige), j’ai lu le code pour voir si c’était comme pour le reste testé. Je n’ai rien vu en particulier.

Sag mir vor allem, ob meine Methodik nicht richtig ist oder ob es zusätzliche Dinge gibt, die ich zusätzlich berücksichtigen sollte. Wenn ich durch richtiges Vorgehen mehr helfen kann, werde ich mich freuen.

16

Das ist eine gute Nachricht, das Update hat viel verbessert. (Die Akzente, die Fehler, …)
Ich habe dafür gesorgt, dass die Fragen für alle gleich sind, also wenn es für den Regen funktioniert, wird es auch für den Schnee funktionieren.

17

Das ist keine Verschmutzung, im Gegenteil. Alles, was die PR-Review betrifft, sollte in GitHub sein, es ist viel einfacher für den Entwickler, deine Feedback-Einträge einzeln zu nehmen und zu bearbeiten.

Wenn du eine Review machst, starte eine „Review“ und hinterlasse Kommentare im Code, wo du denkst, dass es Probleme gibt:

Screenshot 2021-02-01 at 08.54.54
Screenshot 2021-02-01 at 08.54.541804×682 52.4 KB

Danach, wenn du alle deine Rückmeldungen gemacht hast, kannst du eine kleine Zusammenfassung oben schreiben und auf einen von drei möglichen Status klicken:

  • Kommentar: um einen Kommentar zur PR zu hinterlassen
  • Genehmigen: um zu sagen, dass die PR deiner Meinung nach bereit für die Produktion ist
  • Änderungen anfordern: die PR ist nicht bereit für die Produktion, es gibt Probleme, die behoben werden müssen

Screenshot 2021-02-01 at 08.55.49
Screenshot 2021-02-01 at 08.55.491308×938 77.8 KB

Es ist einfacher für alle, wenn wir die GitHub-Mechanismen nutzen :slight_smile:

Toll! Gute Nachricht dann :slight_smile:

Danke für die Tests, methodisch war es gut. Man muss sich einfach in die Lage des Benutzers versetzen und alles von A bis Z testen und sicherstellen, dass alles wie erwartet funktioniert.

18

Ich auch bitte

19

Dieser PR wurde nicht getestet, ich brauche eure Hilfe dabei:

20

Ein neues PR von @bertrandda braucht eine Review :slight_smile:

Ich verlasse mich auf euch!