refactor(weather): Fix timeout, reduce calls, apply shellcheck

[bmodotdev]

Issue

No issue submitted yet.

Description of Changes

General Overview

Fix Darwin timeout function duration argument. Remove unnecessary API calls to “ipinfo.io”. Confirm to Google Shell style guide and appease shellcheck.

The Darwin timeout function makes use of variable duration which does not exist.

One execution of the ./scripts/weather.sh makes 4 remote API calls, 2 calls to the weather service endpoint wttr.in and 2 calls to the geodata endpoint ipinfo.io to fetch city and region. We should make use of the first weather call for both error checking and data gathering. The calls to the geodata endpoint are not necessary as the weather endpoint provides this functionality. This change results in a single API call each script execution.

Exec's of awk, grep, and xargs should not be necessary, as bash parameter expansion allows us to extract data needed.

General style changes to conform to Google Shell style guide, add function argument descriptions, and changes to appease shellcheck. Reduce scope of globals to local function variables.

Shellcheck output

$ shellcheck --severity style scripts/weather*

perl -e 'alarm shift; exec @ARGV' "$duration" "$@"
^-------^ SC2154 (warning): duration is referenced but not assigned.


In scripts/weather.sh line 18:
if $location && [[ ! -z "$fixedlocation" ]]; then
^-- SC2236 (style): Use -n instead of ! -z.


In scripts/weather.sh line 33:
api_response=$(curl -sL wttr.in/${fixedlocation// /%20}\?format="%C+%t$display_weather")
^---------------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
api_response=$(curl -sL wttr.in/"${fixedlocation// /%20}"\?format="%C+%t$display_weather")


In scripts/weather.sh line 38:
echo $api_response
^-----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
echo "$api_response"


In scripts/weather.sh line 54:
unicode=$(forecast_unicode $weather_condition)
^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
unicode=$(forecast_unicode "$weather_condition")


In scripts/weather.sh line 71:
weather_condition=$(echo $weather_condition | awk '{print tolower($0)}')
^----------------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
weather_condition=$(echo "$weather_condition" | awk '{print tolower($0)}')


In scripts/weather_wrapper.sh line 21:
if [ "$(expr ${TIME_LAST} + ${RUN_EACH})" -lt "${TIME_NOW}" ]; then
^--^ SC2003 (style): expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].
^----------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
if [ "$(expr "${TIME_LAST}" + ${RUN_EACH})" -lt "${TIME_NOW}" ]; then


In scripts/weather_wrapper.sh line 23:
$current_dir/weather.sh $fahrenheit $location "$fixedlocation" > "${DATAFILE}"
^----------^ SC2086 (info): Double quote to prevent globbing and word splitting.
^---------^ SC2086 (info): Double quote to prevent globbing and word splitting.
^-------^ SC2086 (info): Double quote to prevent globbing and word splitting.

Did you mean:
"$current_dir"/weather.sh "$fahrenheit" "$location" "$fixedlocation" > "${DATAFILE}"

For more information:
https://www.shellcheck.net/wiki/SC2154 -- duration is referenced but not as...
https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...
https://www.shellcheck.net/wiki/SC2003 -- expr is antiquated. Consider rewr...

Testing

Tested in Ubuntu as below:

$ ./scripts/weather.sh
☀ 70°F Houston, Texas

$ ./scripts/weather.sh true true
☀ 70°F Houston, Texas

$ ./scripts/weather.sh true false
☀ 70°F

$ ./scripts/weather.sh false false
☀ 21°C

$ ./scripts/weather.sh false true
☀ 21°C Houston, Texas

$ ./scripts/weather.sh true true "Los Angeles, California"
☀ 66°F Los Angeles, California

$ ./scripts/weather.sh true false "Los Angeles, California"
☀ 66°F

$ ./scripts/weather.sh false true "Los Angeles, California"
☀ 19°C Los Angeles, California

$ ./scripts/weather.sh "" "" "Los Angeles, California" ;echo
☀ 66°F Los Angeles, California

For the wrapper script, I manually adjusted $INTERVAL to 5, and added an printf "updating cache " to the interval condition, then:

$ /bin/rm /tmp/.dracula-tmux-*; ./scripts/weather_wrapper.sh true false
updating cache ☀ 70°F

$ /bin/rm /tmp/.dracula-tmux-*; ./scripts/weather_wrapper.sh false true "Los Angeles, California"
updating cache ☀ 19°C Los Angeles, California

$ /bin/rm /tmp/.dracula-tmux-*; ./scripts/weather_wrapper.sh true true "Los Angeles, California"; echo; ./scripts/weather_wrapper.sh 
updating cache ☀ 66°F Los Angeles, California
☀ 66°F Los Angeles, California

$ /bin/rm /tmp/.dracula-tmux-*; ./scripts/weather_wrapper.sh false false "Los Angeles, California"; echo; ./scripts/weather_wrapper.sh
updating cache ☀ 19°C
☀ 19°C

I will test on OS X tomorrow at the office.

[bmodotdev]

One thing that wasn't clear to me was the error checking here. I tried finding an example output of the error "====" but couldn't find any in the commit, PR, or in upstream wttr.in project. It's also unclear to me why the second conditional checks for the string "error" when the first conditional sets it; i.e. we could simply return at finding the first error, unless we actually expect the string "error" to appear in addition to "====".

If you can advise on sample error output, I'd be happy to make it more robust and propagate the error message up.

Also, please feel free to let me know if these types of refactors are welcomed or not. I'd be happy to continue with any other scripts or understand if you want to focus on fixes/features only.

Thank you for the cool project!

[bmodotdev]

Ahh, I found #267, which shows the error message as:

Sorry, we are running out of queries to the weather service at the moment.
Here is the weather report for the default city (just to show you what it looks like).
We will get new queries as soon as possible.
You can follow https://twitter.com/igor_chubin for the updates.
======================================================================================

So I suppose the check against "===" is to match this error message. I too would not be confident in exactly which string to match against as I don't know if that string is guaranteed not to change. But "===" is probably not the most obvious method for the reader, nor the most guaranteed string of the message.

I also copied the curl commands, added the include flag, and we see the output has changed since reported in #267

$ curl -isL wttr.in/~Eifal+Tuwer\?format="%C+%t&u"
HTTP/1.1 404 Not Found
Access-Control-Allow-Origin: *
Content-Length: 25
Content-Type: text/plain; charset=utf-u8
Date: Mon, 03 Feb 2025 04:24:56 GMT

Patchy rain nearby +63°F

I think the appropriate thing to do would be to pivot on the status code, especially since this results in leaking user PI. We can see the unknown error is handled by a 404, and surely the status code makes a better contract.

This lead me to also search the upstream project for the capacity text, which lead to the server implementation.

Given this is a potential PI leak, I suggest using curl --fail to drop body and emit a 22 status code, which can allow the consumer to pivot easily. I don't see a reason to keep the body besides debugging.

[bmodotdev]

With status code check:

# master `dd1a7ab`
$ ./scripts/weather.sh true true "Eifal+Tuwer"
☂  63°F Eifal+Tuwer

# PR
$ ./scripts/weather.sh true true "Eifal+Tuwer"
Unknown Location

With this change, I believe we can close #267, which was previously patched via #268 , but appears the API has since changed.

And I believe #308 was a patch to handle the 503 capacity error message, though I am still confused by the comment that was added.

[Theoreticallyhugo]

hi @bmodotdev, you're truly doing gods work. I've been planning on refactoring, but never got around to do it, so I'm very happy you're helping us.
please feel free to continue with refactors like this!
your work is much appreciated :D

[bmodotdev]

Thank you!

I just ran this on OSX, and got an error regarding locale:

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
    LC_ALL = "C.utf8"
[... etc ...]
are supported and installed on your system.
perl: warning: Falling back to a fallback locale ("en_US.UTF-8")

I thought LC_ALL=C.utf8 was a strange value, but did not want to touch it without reason or knowing more. Checking the rest of the project:

$ git grep -hP 'LC_ALL=\S+'|sort|uniq -c
1 export LC_ALL=C.utf8
25 export LC_ALL=en_US.UTF-8

Looks like this is the only script that uses that value, so I've updated it.

I also received an error regarding lowercase parameter expansion. OSX is stuck on Bash3.2 because they refused to accept the change from GPLv2 to GPLv3. I've updated it to use tr instead.

I believe this is PR ready for review now.

Edit: Please squash my fixups or I can (not sure if autosquash is enabled or not).