Skip to content

Get sunrise information from openuv#18

Merged
Tairesh merged 4 commits intoTairesh:mainfrom
peterfpeterson:openuv_sunrise
Apr 3, 2026
Merged

Get sunrise information from openuv#18
Tairesh merged 4 commits intoTairesh:mainfrom
peterfpeterson:openuv_sunrise

Conversation

@peterfpeterson
Copy link
Copy Markdown
Contributor

@peterfpeterson peterfpeterson commented Mar 31, 2026

This parses the sunrise information from openuv and adds it to a new struct.

I'm adding the structure for sunrise information in this PR to keep it relatively small. I intend on making a follow-on PR for formatting the information.

I suggest squash merging PRs to keep a cleaner commit history.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the uv_index type from u8 to f64 across the codebase to allow for more precise weather reporting. It also adds detailed models and tests for the OpenUV API. The review feedback addresses a precision formatting issue in the display logic, corrects inconsistent error messages in test assertions, and fixes a duplicate documentation comment.

@Tairesh
Copy link
Copy Markdown
Owner

Tairesh commented Mar 31, 2026

Thanks for contributing this! The sunrise/sunset data from OpenUV is a great addition and I'm looking forward to seeing it displayed in a follow-on PR.

That said, I'd prefer to keep the changes more focused. This PR bundles two distinct things:

  1. UV index precision — changing uv_index from u8 to f64 and displaying it with one decimal place (affects all providers)
  2. Sunrise/sunset parsing scaffolding — the new SunTimes, SunPosition, SunInfo, SafeExposureTime structs and the expanded UvResult, all currently #[allow(dead_code)]

Could you split these into separate PRs? Something like:

  • PR A: uv_index as f64 with 1-decimal display (small, self-contained, reviewable on its own)
  • PR B: the sunrise/sunset parsing structs in openuv.rs (this PR, minus the UV type change)
  • PR C: the actual formatting/display of sunrise/sunset (the follow-on you mentioned)

This makes each change easier to review and reason about independently. The UV precision change in particular touches every provider and deserves its own focused review.

Happy to merge a tighter version of this once it's split up. Thanks again for the work!

@peterfpeterson peterfpeterson marked this pull request as draft March 31, 2026 20:38
@peterfpeterson peterfpeterson marked this pull request as ready for review April 1, 2026 02:11
- Merge duplicate mod test into mod tests, reuse shared fixture
- Use assert_eq! for known-exact float values from JSON parsing
- Replace is_some() + unwrap() with map() in solar_noon assertion
- Remove redundant #![cfg(test)] from tests.rs (main.rs already gates it)
- Add trailing newline to openuv_response.json
@Tairesh
Copy link
Copy Markdown
Owner

Tairesh commented Apr 3, 2026

Thanks for the contribution! I went ahead and pushed a cleanup commit on top of this branch:

  • Merged the duplicate mod test into the existing mod tests and shared the fixture constant
  • Switched float assertions to assert_eq! where the values round-trip exactly from JSON parsing; kept a small epsilon (1e-10) for azimuth/altitude since their JSON precision exceeds what f64 can represent exactly
  • Replaced the is_some() + unwrap() pattern on solar_noon with a single assert_eq! using .map()
  • Removed the redundant #![cfg(test)] inner attribute from tests.rsmain.rs already gates the module with #[cfg(test)], so both together triggered a Clippy duplicated_attributes lint
  • Added a trailing newline to openuv_response.json

Looking forward to the display PR!

@Tairesh Tairesh merged commit 192bf63 into Tairesh:main Apr 3, 2026
4 checks passed
@peterfpeterson peterfpeterson deleted the openuv_sunrise branch April 3, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants