Review of PRs - I need your help!

Development
1

Hello everyone,

I think it would be great to have a small topic where we can tag each other to ask for PR reviews, so here it is :slight_smile:

There is an increasing number of PRs, and I don’t think I can review them all myself. To prevent PRs from being left unanswered, it would be great if we could help each other with the reviews!

Currently, there are a few PRs that need to be reviewed.

I am particularly thinking of:

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

How to review a PR?

Code Review

To review the code, it’s simple: I read all the code and see if anything shocks me in terms of pure code quality. Variable naming, etc


In short, read the code as if you were the developer of the PR, and ask yourself if you would have done it that way. If certain things shock you, add comments to ask for additional information, and if it really doesn’t work, add a comment and mark the review as « Changes requested Â».

Avoid being aggressive/degrading in the comments, the goal is to be positive: we just want to move the PR forward and have the maximum code quality :slight_smile:

Functional Review

When I review a PR, I pull the PR branch by following the commands provided by GitHub:

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

Then, I start the server:

cd server
npm install
npm start

Then the front end:

cd front
npm install
npm start

Then I test everything absolutely: I try to break the integration by doing everything a user would do: enter, exit, re-enter, open the same page many times, to make sure it doesn’t break.

After reading the code, I usually know a bit what will break: For example, many integrations have « scan Â» pages of devices. I want to make sure these pages do not launch a scan every time the page is loaded, otherwise if the user enters the page 10 times and there are 10 network scans in parallel, it’s heavy ^^ When there is a scan page, in my tests I open the scan page 10 times in 10 different tabs and see what happens :smiley: (And often, it breaks aha)

Thanks to everyone who will help me review PRs, it’s also positive to do PR reviews as it is to make PRs :pray:

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

3

Here’s a new PR ready for feedback, the Netatmo PR by @damalgos and @Terdious!

You can test it here:

4

I am currently testing it, we will review with @Terdious on Saturday.

5

Following an initial review with @Tlse-vins, we noticed that some conditions were not tested for for each loops after queries. We resolved the issue, so I will need to do a new push. How does it work when the PR is under review? Do I need to request a review again? Or is there nothing more to do?

6

You need to mark the requests for modifications as resolved on GitHub

7

Ok! Thanks a lot for the reply @VonOx.

8

Re !!

Could someone help me
 I have a normal eslint error
 but I don’t know how to fix it. So you will surely tell me a noob question. But I must not be good at searching because I can’t find the answer. I created a function to fix the repetition remark from @AlexTrovato in the Netatmo PR. However, one of the input parameters (newValue) can be a {number} or a {boolean}. So I didn’t specify anything in @param. It works fine but eslint gives me an error. Is there a way to disable eslint on this or to specify multiple data types for a param. I tried several things, but nothing works. Thanks in advance

Edit: it’s okay I found it ^^ I didn’t push far enough ^^ In case other people are looking, I leave my post and the link to the doc on this point: https://eslint.org/docs/2.0.0/rules/valid-jsdoc

9

Hello,

for multiple types for parameters or return values, you should use |

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

Thanks so much @wazalop!! I just found the answer and edited my post!! Big thanks anyway for your feedback!!

11

Two new PRs to review!

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

I’d appreciate any help testing these! :slight_smile:

12

Hello !!

Following the PR test

I did complete tests of this last one on the FR side and removed EN, trying some slightly buggy stuff (a bit of a mess) and as long as it makes sense, it works well for me. The display of days instead of the time is good. However, the days are displayed in English despite the FR interface. I therefore recommend the following changes to take into account the FR language:

  • In the file front/src/config/i18n/fr.json - line 17 after "save": "Enregistrer" - add:
    "save": "Enregistrer",
    "daysOfTheWeek": {
      "monday": "Lundi",
      "tuesday": "Mardi",
      "wednesday": "Mercredi",
      "thursday": "Jeudi",
      "friday": "Vendredi",
      "saturday": "Samedi",
      "sunday": "Dimanche"
    }
  • In the file front/src/config/i18n/en.json - line 17 after "save": "Save" - add:
    "save": "Save",
    "daysOfTheWeek": {
      "monday": "Monday",
      "tuesday": "Tuesday",
      "wednesday": "Wednesday",
      "thursday": "Thursday",
      "friday": "Friday",
      "saturday": "Saturday",
      "sunday": "Sunday"
    }

In the file front/src/components/boxs/weather/WeatherBox.jsx - line 270 - replace:

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

with:

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

image
image372×410 12.1 KB

Finally, take advantage of this PR to fix the small bug of the wind unit display which in metric unit gives miles/hour instead of km/h:
image. Replace in the file front/src/components/boxs/weather/WeatherBox.jsx also - line 179:

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

with (a small console.log allows us to see that props.units returns either â€č metric â€ș or â€č  â€ș) by adding a space to the units to make it prettier:

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

Functional for me: image

Hopefully, I have been helpful. I’m moving on to the other PR
Best regards, Thomas.

13

Scenes are created for testing. Now we just have to wait.
However, I already have a comment. It is not specified anywhere what needs to be configured in advance to benefit from this scene. Maybe if the house coordinates (I assume that’s the only condition) are not provided, a warning message would be welcome in the trigger box - especially since there is room:

image
image1180×389 13.7 KB

@pierre-gilles:
When testing a long house name, I was able to identify a « bug Â». When you enter a long name, here â€č Maison Test Sunrise-Sunset sans Emplacement â€ș, the UI returns an error that is not appropriate when saving:
image
image884×742 62.8 KB

instead of « Validation error: Your home name is too long Â».
Issue only in FR.
Issue #1055 created. I will try to do the PR as soon as possible.

Edit: the PR is complete and available for review
 I think ^^: PR #1056

14

Hi @Terdious! Thanks for your reviews, next time try to do the review directly on Github, it’s made for that and it’s easier than going back and forth with the forum :slight_smile:

For the PR about the weather, I think you only tested the tip of the iceberg, the bulk of the PR was on the « discussion with Gladys Â» part, there are dozens of new sentences that have been added, and they need to be tested one by one, and checked that they all work. At my last test, that wasn’t the case but normally there have been improvements since!

For the PR about the sun, good remarks. It’s still better when the remarks are put on the PR :slight_smile:

15

Re,

Then

For that, the problem is that I don’t see the request I made in the changes commits page. Therefore, I couldn’t review the files in question directly on Github. But I left the link to the gladys community

image
image928×280 17.9 KB
because I thought that given the length of the post, it would be a bit of pollution on the github thread. However, not knowing the github review practices well yet, if it’s preferable, I will make sure to apply that next time :slight_smile:

For the testing part, I took this as a basis:

image
image1657×673 27.3 KB

Plus the whole response part. So indeed, I couldn’t test everything because I don’t have snow forecast. But for the rest, everything was functional for all the days of the week, tomorrow and the day after tomorrow; and for all types of questions I did the tests with and without accent, with extra spaces between two, with small typos (ex.: « Donn moi la meteo Â» or « Donne le meteo Â») and all the returns were as expected. For the part I couldn’t test (the snow), I read the code to see if it was like the rest tested. I didn’t see anything in particular.

Let me know especially if my methodology is not the right one, or if there are additional things I should get used to doing more. If I can help more by doing it right, it will be a pleasure.

16

That’s good news, the update must have done a lot. (Accents, typos, 
)
I made sure the questions were the same for everyone, so if it works for rain, it will work for snow.

17

This is not pollution, on the contrary. Everything related to PR reviews should be in GitHub; it’s much easier for the developer to take your feedback one by one and address them.

When you do a review, start a review, and leave comments in the code where you think there are issues:

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

Then, once you’ve given all your feedback, you can write a small summary at the top and click on one of three possible statuses:

  • Comment: to make a comment on the PR
  • Approve: to say that the PR is good to go to production
  • Request changes: the PR is not good to go to production, there are issues to fix

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

It’s simpler for everyone if we use GitHub’s mechanisms :slight_smile:

Great! Good news then :slight_smile:

Thanks for the tests, in terms of methodology, it was good. You just need to put yourself in the user’s shoes and test everything from A to Z, and make sure everything works as expected.

18

Me too please

19

This PR has not been tested, I need your help with this:

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

20

A new PR from @bertrandda needs a review :slight_smile:

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

I’m counting on you!