Use fahrenheit when units are imperial in weather-api-widget - #487
Use fahrenheit when units are imperial in weather-api-widget#487samschmalz wants to merge 2 commits into
Conversation
I didn't see a good way to make a single check for units and set a single temperature variable for each type (temp, feelslike, and forecast min/max), since forecast use a loop to set multiple days in advance. Instead, each location that displays a temperature uses an anonymous function as a ternary operator to determine which to use.
Ryuno-Ki
left a comment
Ryuno-Ki
left a comment
There was a problem hiding this comment.
Looks like a good first iteration. Would you be willing to test a PR if I happen to find the time to polish this?
| @@ -605,7 +605,7 @@ local function worker(user_args) | |||
|
|
|||
| widget:set_image(ICONS_DIR .. icon_map[result.current.condition.code] .. day_night_extension .. icons_extension) | |||
| -- TODO: if units isn't "metric", read temp_f instead | |||
There was a problem hiding this comment.
Do you mind removing comments like these?
There was a problem hiding this comment.
Yes, I can filter those out, I forgot to delete them from whenever they got put in. It looks like I need to break up those lines I added so they pass Luacheck as well, so I'll work on those as well before re-submitting.
There was a problem hiding this comment.
The Luacheck is meant to help with a more robust codebase. I figured to have it run before taking another look.
I am myself guilty of submitting failing code so don't sweat it.
9ab404e to
6ef5c9e
Compare
Ryuno-Ki
left a comment
Ryuno-Ki
left a comment
There was a problem hiding this comment.
That looks way nicer. Did you use a tool to handle the formatting?
| -- zh_cmn, mr, pl, pt, pa, ro, ru, sr, si, sk, es, sv, ta, te, tr, uk, ur, vi, | ||
| -- zh_wuu, zh_hsn, zh_yue, zu | ||
|
|
||
| -- default language is ENglish |
There was a problem hiding this comment.
Nitpick (likely fixed by me): English with lowercase N
There was a problem hiding this comment.
I originally used Neovim, which I have set up with kickstart.nvim so it automatically formats code. However, I realized that doing so also touched almost every other line in the file with an invisible change I can't see (probably whitespace). I'd rather not do that because it obscures meaningful changes, so I had to revert and make the formatting changes manually. I did screw up my local git repo in the process, hence the force-push.
There was a problem hiding this comment.
Ah, a fellow NeoVim developer 🧡
An extraneous TODO was left behind after the previous commit, it has been removed for clarity. Several lines were too long to pass Luacheck, so they have been broken up into multi-line statements so they meet style guidelines.
6ef5c9e to
8a3f6b4
Compare
|
Would you like me to fix the additional whitespace at the end of the listed lines, or would you prefer that I don't generate another commit to be reviewed? I'm okay with either option, but I also want to respect your time. |
|
Please make the luacheck happy. The formatting via kickstart.vim should happen in another PR if you have the energy still. |
2 participants
I didn't see a good way to make a single check for units and set a single temperature variable for each type (temp, feelslike, and forecast min/max), since forecast use a loop to set multiple days in advance. Instead, each location that displays a temperature uses an anonymous function as a ternary operator to determine which to use.