search

bogenpirat / MMM-WetterOnline

MagicMirror module to display weather data from WetterOnline
GNU General Public License v3.0
8 stars 3 forks source link

Review and fix #6

Closed KristjanESPERANTO closed 3 weeks ago

KristjanESPERANTO commented 7 months ago

Thanks for this nice module! :slightly_smiling_face:

I reviewed the code and have some change suggestions.

This will also fix #5.

KristjanESPERANTO commented 4 weeks ago

@bogenpirat Did you noticed this PR?

bogenpirat commented 4 weeks ago

sorry for the late response - i did see this back in december, but it slipped my working memory. apologies for that. thanks for the review! i'm just not sure that i agree with some of your changes.

if i see this right, this is basically just everything run through a formatter along with some string interpolation improvements and removing the superfluous import/updating the readme for it, correct? please correct me if i overlooked something - the formatting changes forced me to go over every line and i may have missed something.

removing cheerio - good point, that should happen. i'm not a fan of the formatter changes. i presume that the string interpolation has some performance benefits in JS? it is code that runs so rarely, i don't think that it requires optimization.

then there's this gem: image really?

KristjanESPERANTO commented 4 weeks ago

i'm just not sure that i agree with some of your changes.

No problem. I am ready to shrink the changes back to an acceptable level :slightly_smiling_face:

if i see this right, this is basically just everything run through a formatter along with some string interpolation improvements and removing the superfluous import/updating the readme for it, correct?

Besides that I profoundly changed the getUrl function.

the formatting changes forced me to go over every line and i may have missed something.

I try to adapt the code to the formatting rules of MagicMirror, but that was obviously too much to put it here in one PR. Sorry. I have undone some of the formatting changes to make it a little easier to compare the changes.

i presume that the string interpolation has some performance benefits in JS?

I doubt that it has performance advantages. The approach is rather to write modern JavaScript code. It wouldn't be a problem if I had to undo this as well. Should I?

bogenpirat commented 3 weeks ago

LGTM, thanks again!