Conversation
- Add `is_day` to Weather and bundle location_name/lat/lon into Location - Add night-variant ASCII art and emoji for Clear and PartlyCloudy - Populate `is_day` natively from open_meteo, weather_api, yr, open_weather_map, and weather_bit - Add NOAA solar-altitude fallback (`weather/sun.rs`) for tomorrow_io and world_weather_online - Introduce `weather/enrich.rs` post-provider wrapper that fills missing `is_day` and `uv_index`; lifts OpenUV calls out of individual providers - Fix world_weather_online: add `includelocation=yes`, parse `nearest_area` for coordinates, share name shortener with tomorrow_io, trim trailing space in description
There was a problem hiding this comment.
Code Review
This pull request introduces day/night awareness by adding an "is_day" field to the Weather model and updating weather providers to populate it. It also replaces the "location_name" string with a structured Location object. A new enrich module is added to calculate daytime status using solar altitude algorithms in sun.rs or extract it from various weather providers. Display logic in formatter.rs and icons.rs now uses this information to show appropriate icons and emojis for night time. A potential issue was identified in the solar altitude calculation where floating-point precision could lead to NaN results in asin(), and a fix was suggested.
Floating-point rounding can push sin_alt slightly outside [-1, 1], making asin() return NaN at poles or exact solstice/equinox times.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Render distinct ASCII art and emoji for
ClearandPartlyCloudyconditions when it is night-time at the queried location, populated from each provider's API where available and falling back to a local solar-altitude calculation otherwise.Weather.is_day: Option<bool>(also surfaced in JSON output);location_name,latitude,longitudebundled into a singleWeather.location: Locationfield.src/weather/sun.rs— pure NOAA low-precision solar altitude function (no I/O, no API key).src/weather/enrich.rs— post-provider wrapper that fills missingis_day(fromsun) anduv_index(from OpenUV); single integration point inApp::fetch_with_fallbackso live mode picks it up automatically.is_dayfor:open_meteo(is_dayquery var),weather_api(is_dayfield),yr(_day/_nightsuffix onsymbol_code),open_weather_map(d/nsuffix on icon code),weather_bit(podfield).tomorrow_ioandworld_weather_online.world_weather_onlinefix: addincludelocation=yes, parsenearest_areafor coordinates, share name shortener withtomorrow_io, trim trailing space in description.open_meteo,open_weather_map,yr) into the shared enrich wrapper.Test Plan
just checkpasses (fmt, build, 141 tests, clippy pedantic)tests/data/wwo_response.jsonwith parsing test (file had zero parsing tests before)is_day: false, Tokyois_day: true), tomorrow_io (Batumi shortener still works after refactor)