[WIP] Weather module #504
[WIP] Weather module #504
Conversation
There was a problem hiding this comment.
Hello!
Merci pour la PR! Très beau travail, j'aime beaucoup le design que tu proposes 👏
Je t'ai mis dans cette review plusieurs feedback. N'y voit rien de négatif, juste des commentaires pour améliorer cette PR avant de la merger :)
Un feedback plus général:
Serait-il possible d'ajouter une option pour activer la box plus complète que tu as créé?Je pense que les deux boxs ont une raison d'exister (une box minimale, et une box complète). Tout le monde ne veut pas forcément les prévisions météos complète sur le dashboard. Dans l'UI de personnalisation du dashboard, on pourrait mettre un sélection radio genre ("box complète"/"box minimale")
| * @example | ||
| * const icon = translateWeatherToFeIcon(weather); | ||
| */ | ||
| const translateWeatherToFeIcon = (weather) => { |
There was a problem hiding this comment.
Je ne suis pas pour coupler l'API de météo au frontend.
Dans ce front on utilise feather icons, mais dans une appli mobile on pourrait utiliser une autre gamme d'icône par exemple. Pour moi l'API de météo doit renvoyer la météo dans un format simple et standard, et ensuite c'est au frontend de gérer la correspondance entre météo et UI/icônes, pas le backend.
Cette API est utilisée à d'autres endroits!
There was a problem hiding this comment.
Oui, mais mais la on ne couple pas au front ou a FE, on change juste la chaine de caratères renvoyée par l'API dans un format compatible (mais non dépendant) de FE.
Si on veut utiliser ce service dans un autre front/app avec une autre libraire que FE, on pourra remettre la fonction de parsing initiale (que j'ai supprimé du front).
En faisant ainsi, on améliore les perfs actuelles en supprimant un bloc de conditions et de traitements, sans dommager les éventuels autres front/app, qui, s'ils n'utilisent pas FE, devront de toute façon parser cette chaine de caractères.
There was a problem hiding this comment.
Si je ne m'abuse la fonction s'appelle bien translateWeatherToFeIcon non? ^^ "ToFeIcon". Donc si, dans cette PR il y a coupling des deux. Pour moi il ne devrait pas y avoir côté serveur de mention de feather icons, on doit renvoyer un attribut weather qui est neutre ("rain", "sunny", des trucs comme ça)
Cette API sert à plein de choses, pas juste le frontend!
Dans les scénarios, tu peux faire "si il est 11H ET que le temps est ensoleillé ALORS ouvrir le store". Ce serait un peu illogique de faire un test sur if "fe-rain", non? Plutôt if "rain" ?
| return ( | ||
| <div style={{ width: '10%', margin: '0.25em 1.25%' }}> | ||
| <p style={{ margin: 'auto', textAlign: 'center', fontSize: '10px', color: 'grey' }}> | ||
| {format24Hours(new Date(hour.datetime).getHours())}h |
There was a problem hiding this comment.
Essaie de privilégier la librairie de formatage des heures qu'on utilise déjà et qui est testée solidement, pas la peine de créer une fonction ici :)
| dataToReturn.weather = 'unknown'; | ||
| if (result.alerts) { | ||
| dataToReturn.alert = { | ||
| title: result.alerts[0].title, |
There was a problem hiding this comment.
Est-on sûr que si result.alerts exist, le tableau est rempli?
There was a problem hiding this comment.
Oui, tous les champs de l'object alerts sont required (cf doc darksky)
There was a problem hiding this comment.
Oui mais est-il possible d'avoir un tableau vide?
Style:
{
alerts: []
}
La documentation DarkSky ne spécifie pas que le tableau est plein si il est précisé. Un test sur la taille du tableau en plus de son existence fera sens avant d'aller piocher dans le tableau.
There was a problem hiding this comment.
https://darksky.net/dev/docs#alerts
alerts est optionnel, mais s'il existe, toutes les variables sont required
There was a problem hiding this comment.
J'avais bien lu la doc, et de mon interprétation ils parlent des objets présent dans le tableau: oui, si le tableau a une entrée, alors l'entrée possède tous ces attributs. Par contre le tableau n'a pas nécessairement d'entrée. Je mettrais un test pour être sur.
| weather: 'fe-cloud', | ||
| }, | ||
| ], | ||
| temperature: '54.87', |
There was a problem hiding this comment.
Pourquoi la température est un string?
There was a problem hiding this comment.
la fonction ToFixed(N) (utilisée pour forcer un nombre à N décimales après la virgule) renvoie une chaine de caractères (bizarrerie de JS).
C'est chelou mais vu que ça n'impacte pas le front, j'ai préféré laissé comme ça plutôt que de forcer une conversion en nombre qui aurait été inutile (comme dans la première remarque, pour une question de performance tout d'abord, mais aussi de modélisation : on ne fait aucun traitement qui n'apporte pas une plus-value).
There was a problem hiding this comment.
Le front est loin d'être le seul endroit qui utilise cette API!
L'API est utilisée dans les scénarios pour faire des règles "Si la température > XX ALORS YY" par exemple.
Cela n'a pas de sens de renvoyer une température sous forme de string, et si il y a des plus value à renvoyer un number qui peut être comparé à un autre number.
Je ne vois pas en quoi c'est moins performant de faire les choses bien.
Attention, j'ai l'impression que tu design cette API pour ton frontend uniquement. C'est très loin d'être le cas, cette API sert avant tout aux scénarios, dans Telegram, et à plein d'autres endroits.
Le formattage ne doit pas être fait côté serveur, mais côté client à mon sens. Ton toFixed devrait être fait côté client selon moi.
| units: 'si', | ||
| wind_speed: 5.25, | ||
| weather: 'cloud', | ||
| wind_speed: '5.25', |
There was a problem hiding this comment.
La vitesse du vent est un string aussi?
There was a problem hiding this comment.
Idem que pour la température, j'utilise cette fonction pour arrondir ces 2 champs, ainsi que la température apparente.
There was a problem hiding this comment.
Idem que pour la réponse précédente.
Codecov Report
@@ Coverage Diff @@
## master #504 +/- ##
==========================================
+ Coverage 91.14% 91.54% +0.39%
==========================================
Files 334 334
Lines 3794 3784 -10
==========================================
+ Hits 3458 3464 +6
+ Misses 336 320 -16
Continue to review full report at Codecov.
|
|
J'ai ajusté selon tes remarques et ajouté une option de choix dans la configuration pour garder une version basique ou avancée. Par contre, je suis bloqué sur le test unitaire du service (j'ai volontairement laissé un test non réussi), car je ne vois pas comment changer une variable du service pour indiquer quelle version utiliser. Pour être plus clair, une variable service permet d'indiquer s'il faut traiter la réponse selon un affichage basique ou avancé, et je ne vois pas comment simuler cette variable dans le test. |
|
Regarde du côté des tests du service zwave. |
|
La configuration du mode d'affichage (basic ou avancé) se fait maintenant dans la configuration de la box, et non dans l'encart intégration du service (ce qui n'était pas très logique)
|
C'est très propre comme ça! Beau boulot :) As tu résolu tes problèmes de tests? Il manque un peu de code coverage à ce que je vois. |
|
En fait entre temps, j'ai découvert l'encart chat, que je n'avais vraiment vraiment regardé jusque la J'ai vu que ma dernière PR cassait la demande de météo via le chat, sans que les tests me l'indiquent, donc j'ai commencé par fixer ça, et pendant que j'y étais, j'ai ajouté une fonctionnalité pour demander via le chat des prévisions météo pour une date future. J'ai bien galéré avec les dates JS, les différents formats pour demander une date, et le module brain, mais ça a été assez instructif ) Je viens tout juste de terminer le dev (tout fonctionne, me reste juste un peu d'optimisation de code/perfs et d'UX), et le chat répond maintenant aux questions du type :
Concrètement, le service météo peut maintenant renvoyer la prévision météo selon un timestamp passé en option (j'ai aussi fix l'option offset existante). L’idée finale est de pouvoir utiliser ce service simplement dans d'autres services
Sinon pour les tests, travailles tu en TDD ? j'ai pas vraiment l'impression en voyant tes tests, et j'ai un peu le sentiment que ceux ci sont plus développés pour avoir un code coverage le plus proche possible des 100%, que pour tester tous les uses cases (notamment d'erreurs) pouvant arriver. Bref, je suis pas vraiment à l'aise avec les tests pour le moment, je pense passer mon week end dessus pour bien comprendre et finaliser ce module (je t'avoue que j'avais clairement pas prévu de passer autant de temps sur celui ci pour au final une plus value qui est plus de l'ordre du confort que de la nécessité) |
There was a problem hiding this comment.
Super cool tout ça! C'est des vrais features utiles, c'est vraiment top. Beau boulot 👏
Sinon pour les tests, travailles tu en TDD ?
Alors oui je travaille principalement en TDD, après je t'avoue que ce service je l'ai développé le jour même de la sortie de l'alpha en urgence, ça m'étonne pas qu'il y a encore des petites approximations dans les tests. Ce service n'a pas été développé en TDD.
|
Ok je comprends mieux. Je ferai les tests ce week end, et je pullerai les devs pour le chat une fois que tous les tests seront validés. |
|
Top! Tiens moi au courant et n'hésite pas en cas de questions :) |
|
Ok, je viens de balancer la dernière version.
N'hésites pas a faire une une code review ;) |
There was a problem hiding this comment.
Hello!
Très beau boulot, on commence à voir le bout! :)
Pour moi quasi pas de blocages sur cette PR à part des petits problèmes mineurs.
Je t'ai tout mis en commentaire.
J'adopte volontairement un ton naïf dans certains commentaires pour questionner certains choix, rien de méchant c'est juste pour se poser les bonnes questions et pour éviter qu'on fixe des conventions implicites dans Gladys qu'on ne veut pas forcément.
| @@ -193,6 +193,7 @@ const INTENTS = { | |||
| }, | |||
| WEATHER: { | |||
| GET: 'intent.weather.get', | |||
| GET_PREVISIONS: 'intent.weather.getPrevisions', | |||
There was a problem hiding this comment.
Attention à la convention, ici pas de camelCase dans les clés d'intents, on met des tirets!
| const { latitude, longitude, language, units, offset, datetime } = optionsMerged; | ||
| let timestamp = ''; | ||
| if (offset !== 0) { | ||
| timestamp = `, ${moment() |
There was a problem hiding this comment.
Petite question, as tu essayé d'utiliser daysjs au lieu de moment ici?
Une des philosophies de la v4 est de rester le plus léger possible au niveau des librairies utilisées (quand c'est possible!) Moment est connu pour être un monstre assez lourd, et j'ai l'impression qu'ici dayjs pourrait faire le boulot ici. Qu'en pense tu?
There was a problem hiding this comment.
Oui, je me suis fait la même remarque sur moment, mais dayjs ne semble pas gérer les timezones, et me sortait des timestamp erronés.
There was a problem hiding this comment.
Ah? daysjs ne gère pas les timezones? Même avec quelque chose comme ça :
?
| return { icon: 'cloud', summary: 'cloudy' }; | ||
| } | ||
| if (weather.search('night') !== -1 && (datetime && sunrise && sunset && (datetime < sunrise || datetime > sunset))) { | ||
| return { icon: 'night', summary: 'clear night' }; |
There was a problem hiding this comment.
Est-ce qu'on pourrait avoir des clés standardisée et sans espace ni majuscule? ("clear-night" ici)
J'ai vu dans les fichiers de tests que les summary n'était pas les mêmes qu'ici, passer par un fichier de consts dans le service darsky permettrait d'éviter ce genre d'erreur d'inattention :)
There was a problem hiding this comment.
Le summary ici n'est pas le même que celui ci retourne par l'API Darksky.
Celui de Darksky est inutilisable car toujours en anglais et non documenté.
Celui ci, basé sur l’icône renvoyée par Darksky est utilisé dans les réponses textuelles du chat (c'est donc une valeur affichée a l'utilisateur).
J'ai supprime le summary de darksky que je renvoyais dans les prévisions horaires pour éviter la confusion (d'autant plus, qu'au final, je ne l'utilisais pas)
Par contre, tu as raison, il faudrait mettre ce texte dans le dossier config i18n, pour afficher dans la langue de l'utilisateur une fois que le français sera pris en charge par Gladys.
Mais je n'arrive pas a récupérer la langue de l'utilisateur dans ma fonction, comment faire ?
La commande gladys.stateManager.get('system', 'SYSTEM_LANGUAGE') utilisée dans le service Hue ne me renvoie rien car l'object gladys.stateManager.system est vide
| switch (classification.intent) { | ||
| case 'weather.get': | ||
| weather = await this.get(house); | ||
| context.temperature = weather.temperature; | ||
| context.units = weather.units === 'si' ? '°C' : '°F'; | ||
| await this.messageManager.replyByIntent(message, `weather.get.success.${weather.weather}`, context); | ||
| break; | ||
| case 'weather.getPrevisions': | ||
| if (context.time) { | ||
| house.target = 'hourly'; |
There was a problem hiding this comment.
Pourquoi faire une mutation sur un paramètre de cette fonction? Cela n'a pas de sens que la maison est un attribut "target"?
There was a problem hiding this comment.
Parce que selon la requete, cette variable peut avoir 3 valeurs : currently, hourly and daily.
Et le retour de l'API est différent selon ces 3 uses cases.
Le nom target est peut être pas le plus pertinent, mais j'avoue manquer d'inspiration ^^
There was a problem hiding this comment.
En fait mon problème c'est surtout que tu fais une mutation sur l'objet "house", ça n'a pas de sens! Pourquoi une maison aurait un attribut target?
| dataToReturn.datetime = new Date(dataPoint.time * 1000); | ||
| dataToReturn.units = options.units; | ||
| dataToReturn.wind_speed = dataPoint.windSpeed; | ||
| dataToReturn.time_sunrise = new Date(result.daily.data[0].sunriseTime * 1000); |
There was a problem hiding this comment.
Attention, en anglais:
- "heure du lever de soleil" = "sunrise time"
- "heure du coucher de soleil" = "sunset time"
There was a problem hiding this comment.
Je sais, c'est un choix.
Je préférè regrouper les valeurs comme object_attribut plutôt que attribut_object
Ça permet de mutualiser les valeurs de même famille (mais pas forcément du même object) avec le même préfixe.
ça me parait plus logique, plus instinctif, mais surtout, ça aide pas mal pour l’autocomplétion des IDE
Par exemple, si je me souviens plus de tous les attributs wind que j'ai, juste en tapant "wind_", l'IDE affiche la liste des variables existantes. Sans ça, il faut scroller pour rechercher le nom des variables dans le code.
| return { icon: 'night', summary: 'clear night' }; | ||
| } | ||
| if (weather.search('rain') !== -1) { | ||
| return { icon: 'rain', summary: 'raining' }; |
There was a problem hiding this comment.
Autre remarque, j'ai du mal à comprendre la différence entre "icon" et "summary", j'ai l'impression que les deux renvoient toujours la même chose avec juste une différence grammaticale.. Si les deux sont liés, pourquoi en avoir deux?
There was a problem hiding this comment.
Icon est utilise pour l'affichage graphique et summary pour une réponse textuelle
| switch (classification.intent) { | ||
| case 'weather.get': | ||
| weather = await this.get(house); | ||
| context.temperature = weather.temperature; | ||
| context.units = weather.units === 'si' ? '°C' : '°F'; | ||
| await this.messageManager.replyByIntent(message, `weather.get.success.${weather.weather}`, context); | ||
| break; | ||
| case 'weather.getPrevisions': | ||
| if (context.time) { | ||
| house.target = 'hourly'; |
There was a problem hiding this comment.
En fait mon problème c'est surtout que tu fais une mutation sur l'objet "house", ça n'a pas de sens! Pourquoi une maison aurait un attribut target?
| const { latitude, longitude, language, units, offset, datetime } = optionsMerged; | ||
| let timestamp = ''; | ||
| if (offset !== 0) { | ||
| timestamp = `, ${moment() |
There was a problem hiding this comment.
Ah? daysjs ne gère pas les timezones? Même avec quelque chose comme ça :
?
|
Hello :) De retour de congés, je reprend les PR. Du nouveau sur celle-ci ? |
|
Je crois que le service darksky n'est plus d'actualité, non ? |
|
Je ferme cette PR en faveur de #961 |
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm teston both front/server)npm run eslinton both front/server)npm run prettieron both front/server)NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Screenshots
Without and with forecast alert