Review des PR - J'ai besoin de vous!

pierre-gilles · Janvier 18, 2021, 4:41

Salut à tous,

Je pense qu’il serait bien qu’on ait un petit sujet où on puisse se tag pour se demander des review de PR, alors le voilà

Il y a un nombre croissant de PR, et je ne pense pas pouvoir tout review moi-même. Pour pas que des PR se retrouvent sans réponse, il serait cool qu’on puisse s’entraider sur les review !

Actuellement il y a quelques PR qui demandent à être review.

Je pense notamment à :

https://github.com/GladysAssistant/Gladys/pull/1047

https://github.com/GladysAssistant/Gladys/pull/1046

https://github.com/GladysAssistant/Gladys/pull/1026

Comment review une PR?

La review de code

Pour review le code, c’est simple: je lis tout le code et je vois si des trucs me choque, en terme de qualité de code pur. Nommage des variables, etc…

En gros, lisez le code comme si vous étiez développeur de la PR, et demandez vous si vous l’auriez fait comme ça. Si certaines choses vous choquent, mettez des commentaires pour demander des renseignements supplémentaires, et si vraiment ça ne va pas, mettez un commentaire et mettez la review comme “Changes requested”.

Evitez d’être agressif/dégradant dans les commentaires, l’objectif est d’être positif: on veut juste faire avancer la PR et avoir une qualité de code maximale

La review fonctionnelle

Lorsque je review une PR, je pull la branche de la PR en suivant les commandes fournies par GitHub:

Ensuite, je lance le serveur:

cd server
npm install
npm start

Puis le front:

cd front
npm install
npm start

Puis je test absolument tout: j’essaie de casser l’intégration en faisant tout ce qu’un utilisateur ferait: entrer, sortir, re-entrer, ouvrir plein de fois la même page, pour être sur que ça ne casse pas.

Après avoir lu le code, en général je sais un peu ce qui va casser: Par exemple beaucoup d’intégrations ont des pages de “scan” de périphériques. Je veux être sur que ces pages ne lancent pas un scan à chaque chargement de la page, sinon si l’utilisateur rentre 10 fois sur la page et qu’il y a 10 scans du réseau en parallèle, ça fait lourd ^^ Quand il y a une page de scan, dans mes tests j’ouvre 10 fois la page de scan dans 10 onglet différent et je vois ce que ça fait (Et souvent, ça casse aha)

Merci à tous ceux qui m’aideront à review des PR, c’est aussi positif de faire des review de PR que de faire des PR

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

pierre-gilles · Janvier 22, 2021, 4:14

Hop, une nouvelle PR prête qui a besoin de retours, la PR Netatmo par @damalgos et @Terdious !

A tester ici :

https://github.com/GladysAssistant/Gladys/pull/1015

Tlse-vins · Janvier 22, 2021, 4:57

Je suis justement entrain de la tester, on fait le point avec @Terdious samedi.

Terdious · Janvier 23, 2021, 5:24

Suite a une première review avec @Tlse-vins, on a pu voir que certaines conditions n’étaient pas testée pour les for each apres requêtes. On a résolu le problème du coup je vais devoir faire un nouveau push. Comment ça se passe lorsque la PR est en review ? Je dois relancer la demande de review ? Ou rien a faire de plus ?

VonOx · Janvier 23, 2021, 5:45

Il faut marquer comme corrigé les demandes de modifications sur github

Terdious · Janvier 23, 2021, 6:54

Ok !! Merci beaucoup pour la réponse @VonOx.

Terdious · Janvier 24, 2021, 9:02

Re !!

Quelqu’un pourrait-il m’aider… j’ai une erreur eslint … normale … mais je ne sais comment la corriger. Alors vous me direz sûrement une question noob. Mais je ne dois pas être doué pour chercher car je ne trouve pas la réponse. J’ai créé une fonction pour corriger la remarque de répétition d’ @AlexTrovato dans la PR Netatmo. Or l’un des paramètres d’entrée (newValue} peut être un {number} ou un {boolean}. Du coup je n’ai rien spécifié dans @param. Ca marche bien mais eslint me fait une erreur. Y a-t-il moyen de désactiver eslint là-dessus ou bien de spécifier plusieurs type de donnée sur un param. J’ai tenté plusieurs choses, mais rien n’y fait.
Merci par avance

Edit : c’est bon j’ai trouvé ^^ je n’avais pas poussé assez ^^ Au cas ou d’autres personnes rechercherai, je laisse mon poste et le lien de la doc sur ce point : Rule valid-jsdoc - ESLint - Pluggable JavaScript linter

wazalop · Janvier 24, 2021, 9:15

Salut,

pour les cas de plusieurs type pour les params ou retour faut utiliser |

@param {(string|string[])} [somebody=John Doe] - Somebody's name, or an array of names.

Terdious · Janvier 24, 2021, 9:16

Merci beaucoup @wazalop !! Je viens juste de trouver la réponse et d’éditer mon post !! Un grand merci en tout cas pour ton retour !!

pierre-gilles · Janvier 29, 2021, 1:19

Et deux nouvelles PRs à review !

https://github.com/GladysAssistant/Gladys/pull/1026

https://github.com/GladysAssistant/Gladys/pull/1001

Je suis preneur de tout coup de main sur celles-là pour tout tester

Terdious · Janvier 30, 2021, 6:56

Hello !!

Suite à essai de la PR

J’ai fait les tests complets de cette dernière côté FR et ôté EN, en tentant quelques truc un peu buggé (écriture un peu à l’arrache) et tant que ça reste censé, ça marche bien pour moi. L’affichage des jours à la place de l’heure est bon. Malgré tout, les jours s’affiche en Anglais malgré l’interface FR. Je préconise donc les changement suivants pour prendre en compte la langue FR :

Dans le fichier front/src/config/i18n/fr.json - ligne 17 après "save": "Enregistrer" - ajout de :

    "save": "Enregistrer",
    "daysOfTheWeek": {
      "monday": "Lundi",
      "tuesday": "Mardi",
      "wednesday": "Mercredi",
      "thursday": "Jeudi",
      "friday": "Vendredi",
      "saturday": "Samedi",
      "sunday": "Dimanche"
    }

Dans le fichier front/src/config/i18n/en.json - ligne 17 après "save": "Save" - ajout de :

    "save": "Save",
    "daysOfTheWeek": {
      "monday": "Monday",
      "tuesday": "Tuesday",
      "wednesday": "Wednesday",
      "thursday": "Thursday",
      "friday": "Friday",
      "saturday": "Saturday",
      "sunday": "Sunday"
    }

Dans le fichier front/src/components/boxs/weather/WeatherBox.jsx - ligne 270 - remplacer :

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

par :

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

Fonctionnel chez moi :

Enfin profiter de cette PR pour corriger le petit bug de l’affichage de l’unité de vent qui en unité metric donne des miles/heure au lieu des km/h :
. Remplacer dans le fichier front/src/components/boxs/weather/WeatherBox.jsx également - ligne 179 :

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

par (un petit console.log nous permet de voir que props.units renvoi soit 'metric soit ‹ ›) en rajoutant un espace aux unités pour que ce soit plus joli :

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

Fonctionnel chez moi :

En espérant avoir été utile. Je passe à l’autre PR
Cordialement, Thomas.

Terdious · Janvier 30, 2021, 8:00

Alors les scènes sont créées pour le test. Il n’y a plus qu’à attendre.
Toutefois déjà une remarque. Il n’est précisé nulle part ce qui doit-être configuré au préalable pour pouvoir profiter de cette scène. Peut-être que si les coordonnées de la maison (je suppose que c’est la seule condition) ne sont pas renseignés, un message d’alerte serait le bien venu dans la box trigger - d’autant qu’il y a de la place :

@pierre-gilles :
En faisant un essai de nom de maison à rallonge, j’ai pu y soulever un « bug », lorsque on met un nom à rallonge, ici ‹ Maison Test Sunrise-Sunset sans Emplacement ›, l’UI nous rapport une erreur qui ne convient pas lorsqu’on sauvegarde :
au lieu de « Erreur de validation : Le nom de votre maison est trop long ».
Problème seulement en FR.
Issue #1055 créée. Je vais essayer de faire la PR au plus vite.

Edit : la PR est terminée et disponible en review … je pense ^^ : PR #1056

pierre-gilles · Janvier 31, 2021, 9:52

Salut @Terdious! Merci pour tes reviews, la prochaine fois essaie de faire la review directement sur Github, c’est fait pour ça et c’est plus simple que de faire l’aller retour avec le forum

Pour la PR sur la météo, je crois que tu n’as testé que la face émergé de l’iceberg, le plus gros de la PR était sur la partie “discussion avec Gladys”, il y a des dizaines de nouvelles phrases qui ont été ajoutée, et il faut les tester une par une, et vérifier qu’elles fonctionnent tout. A mon dernier test, ce n’était pas le cas mais normalement il y a eu des améliorations depuis !

Pour la PR sur le soleil, bonnes remarques. La encore, c’est mieux quand les remarques sont mises sur la PR

Terdious · Janvier 31, 2021, 11:41

Re,

Alors

Pour ça, le problème c’est que je nois pas dans la page des changements commits la demande que j’ai faite. Du coup je ne pouvais pas review les fichiers en questions directement sur Github. Mais j’y ai laissé le lien vers gladys community

car je me disais que vu la longueur du post, ça ferai un peu pollution sur le fil github. Pour autant, ne connaissant pas encore trop les pratiques sur les review github, si c’est préférable alors je ferais en sorte d’appliquer ça la prochaine fois

Pour la partie testing, j’ai pris pour appui ceci :

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.

Dis moi surtout si ma méthodologie n’est pas la bonne, ou s’il y a des choses supplémentaire que je devrais prendre l’habitude en plus. Si je peux apporter plus de mon aide en le faisant comme il faut, ce sera un plaisir.

Tlse-vins · Janvier 31, 2021, 12:19

C’est une bonne nouvelle, la mise à jour a dû faire beaucoup. (Les accents, les fautes, …)
J’avais fait en sorte que les questions soient les mêmes pour tous donc si ça marche pour la pluie, ça marchera pour la neige.

pierre-gilles · Février 1, 2021, 7:59

Ce n’est pas de la pollution, au contraire. Tout ce qui concerne les review de PR doit être dans GitHub, c’est beaucoup plus simple pour le développeur de prendre un par un tes feedbacks et les traiter.

Quand tu fais une review, fait « start a review », et met les commentaires dans le code là ou tu penses qu’il y a des soucis :

Ensuite, une fois que tu as fais tous tes retours, tu peux écrire un petit recap en haut et cliquer sur 3 statuts possible:

Comment: pour faire un commentaire sur la PR
Approve: dire que pour toi la PR est bonne à partir en prod
Request changes: la PR n’est pas bonne à partir en prod, il y a des soucis à fixer

C’est plus simple pour tout le monde si on utilise les mécanismes GitHub

Génial ! Bonne nouvelle alors

Merci pour les tests, niveau méthodologie c’était bon. Il faut juste se mettre à la place de l’utilisateur et tester tout de A à Z, et être sur que tout fonctionne comme prévu.

AlexTrovato · Février 3, 2021, 6:13

Moi aussi SVP

https://github.com/GladysAssistant/Gladys/pull/1027

pierre-gilles · Février 19, 2021, 9:52

Cette PR n’a pas été testée, j’ai besoin de vous là dessus:

https://github.com/GladysAssistant/Gladys/pull/1026

pierre-gilles · Mars 8, 2021, 2:55

Une nouvelle PR de @bertrandda a besoin d’une review

https://github.com/GladysAssistant/Gladys/pull/1091

Je compte sur vous !