Conversation
Contributor
Reviewer's GuideImproves README documentation and developer onboarding while significantly refactoring tests around external APIs to use deterministic mocks and aligning cache names between tests and implementation for the tide/surf data API. Sequence diagram for tide and surf forecast request with caching and external APIsequenceDiagram
actor Surfer
participant FastAPI as FastAPI_server
participant ApiModule as api_module
participant ForecastCache as forecast_cache
participant OpenMeteo as OpenMeteo_API
Surfer->>FastAPI: GET /?loc=malibu&fc=3
FastAPI->>ApiModule: forecast(lat,long,decimal,days=3)
ApiModule->>ForecastCache: check_cache(lat,long,decimal,days)
alt cache_hit
ForecastCache-->>ApiModule: cached_forecast_data
ApiModule-->>FastAPI: cached_forecast_data
FastAPI-->>Surfer: formatted_surf_forecast
else cache_miss
ForecastCache-->>ApiModule: miss
ApiModule->>OpenMeteo: weather_api(url,params)
OpenMeteo-->>ApiModule: raw_forecast_response
ApiModule->>ApiModule: parse_and_normalize_forecast
ApiModule->>ForecastCache: store(lat,long,decimal,days,forecast_data)
ApiModule-->>FastAPI: forecast_data
FastAPI-->>Surfer: formatted_surf_forecast
end
Class diagram for updated cache objects and API functions in tide/surf moduleclassDiagram
class TTLCache {
+int maxsize
+int ttl
+get(key)
+set(key,value)
}
class api_module {
<<module>>
+_TTL : int
+_ocean_cache : TTLCache
+_uv_cache : TTLCache
+uv_history_cache : TTLCache
+ocean_history_cache : TTLCache
+_wind_temp_cache : TTLCache
+_rain_cache : TTLCache
+forecast_cache : TTLCache
+_hourlyforecast_cache : TTLCache
+_ocean_lock : Lock
+get_uv_history(lat,long,decimal,unit) str
+ocean_information(lat,long,decimal,unit) list~float~
+ocean_information_history(lat,long,decimal,unit) list~float~
+get_rain(lat,long) tuple~float,float~
+forecast(lat,long,decimal,days) dict
+get_hourly_forecast(lat,long,days,unit) dict
}
class Lock {
+acquire()
+release()
}
api_module "1" o-- "1" TTLCache : uses_for_ocean_data
api_module "1" o-- "1" TTLCache : uses_for_uv
api_module "1" o-- "1" TTLCache : uses_for_uv_history
api_module "1" o-- "1" TTLCache : uses_for_ocean_history
api_module "1" o-- "1" TTLCache : uses_for_wind_temp
api_module "1" o-- "1" TTLCache : uses_for_rain
api_module "1" o-- "1" TTLCache : uses_for_daily_forecast
api_module "1" o-- "1" TTLCache : uses_for_hourly_forecast
api_module "1" o-- "1" Lock : synchronizes_cached_calls
api_module ..> TTLCache : creates_instances
api_module ..> Lock : uses_for_thread_safety
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The rename of
_forecast_cache/_ocean_history_cacheto publicforecast_cache/ocean_history_cachechanges their visibility; if the goal is only to clear them in tests, consider keeping them private and exposing a small reset helper or usingcachetools.cached.cache_clear()in tests to avoid widening the public surface. - The
_hourlyforecast_cachename is inconsistent with the other cache variables and withget_hourly_forecast; consider renaming it to_hourly_forecast_cachefor readability and to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The rename of `_forecast_cache`/`_ocean_history_cache` to public `forecast_cache`/`ocean_history_cache` changes their visibility; if the goal is only to clear them in tests, consider keeping them private and exposing a small reset helper or using `cachetools.cached.cache_clear()` in tests to avoid widening the public surface.
- The `_hourlyforecast_cache` name is inconsistent with the other cache variables and with `get_hourly_forecast`; consider renaming it to `_hourly_forecast_cache` for readability and to avoid confusion.
## Individual Comments
### Comment 1
<location path="src/api.py" line_range="35-38" />
<code_context>
-_forecast_cache = TTLCache(maxsize=300, ttl=_TTL)
-_hourly_forecast_cache = TTLCache(maxsize=300, ttl=_TTL)
+forecast_cache = TTLCache(maxsize=300, ttl=_TTL)
+_hourlyforecast_cache = TTLCache(maxsize=300, ttl=_TTL)
_ocean_lock = Lock()
</code_context>
<issue_to_address>
**suggestion (typo):** Align `_hourlyforecast_cache` naming with the other cache variables for clarity.
The new `_hourlyforecast_cache` name removes the underscore between `hourly` and `forecast`, diverging from the previous `_hourly_forecast_cache` and the style of the other caches. This looks more like a typo than an intentional change. Please either keep `_hourly_forecast_cache` or use `hourly_forecast_cache` if it’s meant to be public, to keep naming consistent and readable.
Suggested implementation:
```python
forecast_cache = TTLCache(maxsize=300, ttl=_TTL)
_hourly_forecast_cache = TTLCache(maxsize=300, ttl=_TTL)
```
If `_hourlyforecast_cache` is referenced elsewhere in `src/api.py` or other modules, those references should also be renamed to `_hourly_forecast_cache` to match this change and avoid `NameError`s.
</issue_to_address>
### Comment 2
<location path="tests/test_api.py" line_range="109" />
<code_context>
+ assert isinstance(result[2], str)
+
+
+@patch("src.api._create_openmeteo_client")
+def test_get_uv(mock_create_client):
+ UV_INDEX = 5.0
+ mock_variable = MagicMock()
+ mock_variable.Value.return_value = UV_INDEX # real number so round() works
+
+ mock_current = MagicMock()
+ mock_current.Variables.return_value = mock_variable
+
+ mock_response = MagicMock()
+ mock_response.Current.return_value = mock_current
+
+ mock_client = MagicMock()
+ mock_client.weather_api.return_value = [mock_response]
+ mock_create_client.return_value = mock_client
+
+ result = get_uv(31.41, -84.92, 2, "imperial")
+
+ assert result == UV_INDEX
+ assert isinstance(result, float)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Clear the get_uv cache in this test to avoid flakiness from cached values
Because `get_uv` uses a TTL cache, this test may reuse a value cached by another test with the same arguments, causing flakiness. To keep tests isolated, clear the cache (e.g., `get_uv.cache_clear()`) before calling it here, consistent with how you clear `get_coordinates` and `uv_history_cache`.
```suggestion
get_uv.cache_clear()
result = get_uv(31.41, -84.92, 2, "imperial")
```
</issue_to_address>
### Comment 3
<location path="README.md" line_range="26-35" />
<code_context>
+---
+
+## Table of Contents
+
+- [Usage](#-usage)
+- [Setup](#️-setup)
+ - [Poetry](#how-to-start-locally-with-poetry)
+ - [Docker](#how-to-start-with-docker)
+ - [Environment Variables](#variables)
+ - [Email Server](#email-server)
+ - [MongoDB](#mongodb)
+- [GPT Surf Report](#-gpt-surf-report)
+- [Tech Stack](#-tech-stack)
+- [Contributing](#-contributing)
+- [Contributors](#-contributors)
+
+---
</code_context>
<issue_to_address>
**issue (bug_risk):** Table of Contents anchor links likely do not match the generated heading IDs and may be broken, especially the `#️-setup` anchor.
These anchors (e.g., `#-usage`, `#️-setup`, `#-gpt-surf-report`) likely don’t match the IDs auto-generated from headings like `## 💻 Usage` and `## 🛠️ Setup`. In particular, `#️-setup` has an invisible character after `#`, which will break the link. Please check how your renderer (e.g., GitHub) slugs these headings and update the anchors accordingly (likely `#usage`, `#setup`, `#gpt-surf-report`, etc.), removing the stray character in `#️-setup`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+35
to
38
| _hourlyforecast_cache = TTLCache(maxsize=300, ttl=_TTL) | ||
| _ocean_lock = Lock() | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
suggestion (typo): Align _hourlyforecast_cache naming with the other cache variables for clarity.
The new _hourlyforecast_cache name removes the underscore between hourly and forecast, diverging from the previous _hourly_forecast_cache and the style of the other caches. This looks more like a typo than an intentional change. Please either keep _hourly_forecast_cache or use hourly_forecast_cache if it’s meant to be public, to keep naming consistent and readable.
Suggested implementation:
forecast_cache = TTLCache(maxsize=300, ttl=_TTL)
_hourly_forecast_cache = TTLCache(maxsize=300, ttl=_TTL)If _hourlyforecast_cache is referenced elsewhere in src/api.py or other modules, those references should also be renamed to _hourly_forecast_cache to match this change and avoid NameErrors.
| mock_client.weather_api.return_value = [mock_response] | ||
| mock_create_client.return_value = mock_client | ||
|
|
||
| result = get_uv(31.41, -84.92, 2, "imperial") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Clear the get_uv cache in this test to avoid flakiness from cached values
Because get_uv uses a TTL cache, this test may reuse a value cached by another test with the same arguments, causing flakiness. To keep tests isolated, clear the cache (e.g., get_uv.cache_clear()) before calling it here, consistent with how you clear get_coordinates and uv_history_cache.
Suggested change
| result = get_uv(31.41, -84.92, 2, "imperial") | |
| get_uv.cache_clear() | |
| result = get_uv(31.41, -84.92, 2, "imperial") |
Comment on lines
+26
to
+35
| ## Table of Contents | ||
|
|
||
| - [Usage](#-usage) | ||
| - [Setup](#️-setup) | ||
| - [Poetry](#how-to-start-locally-with-poetry) | ||
| - [Docker](#how-to-start-with-docker) | ||
| - [Environment Variables](#variables) | ||
| - [Email Server](#email-server) | ||
| - [MongoDB](#mongodb) | ||
| - [GPT Surf Report](#-gpt-surf-report) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Table of Contents anchor links likely do not match the generated heading IDs and may be broken, especially the #️-setup anchor.
These anchors (e.g., #-usage, #️-setup, #-gpt-surf-report) likely don’t match the IDs auto-generated from headings like ## 💻 Usage and ## 🛠️ Setup. In particular, #️-setup has an invisible character after #, which will break the link. Please check how your renderer (e.g., GitHub) slugs these headings and update the anchors accordingly (likely #usage, #setup, #gpt-surf-report, etc.), removing the stray character in #️-setup.
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
1 participant
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.
General:
Code:
Summary by Sourcery
Improve documentation for cli-surf usage and configuration, and refactor tests to rely on mocked external services and new cache names for more reliable, deterministic behavior.
Enhancements:
Documentation:
Tests: