Feature: Add Historical Weather Data Comparison #152
Conversation
ocean_data_dict and mapping to accommodate this new function.
Updated ocean_data_dict and the mapping to accommodate the new print_historical_data function.
… information, modify arguments accordingly
…an_information_history API call to fetch data for a specific date
…ng rounding and correcting timezone handling
…ing and data retrieval
…et_uv_history functions and print_historical_data
…mation_history function
…nd print_historical_data
…and comments for clarity
Reviewer's Guide by SourceryThis PR implements historical weather data comparison functionality by adding new API endpoints to fetch data from one year ago, extending existing functions to handle historical data, and creating new display functions. The implementation includes comprehensive error handling, testing capabilities through a testing flag, and proper data formatting. Sequence diagram for fetching historical weather datasequenceDiagram
participant User
participant CLI
participant API
participant OpenMeteo
User->>CLI: Request historical weather data
CLI->>API: Call gather_data with historical flag
API->>OpenMeteo: Fetch UV history
OpenMeteo-->>API: Return UV history data
API->>OpenMeteo: Fetch ocean information history
OpenMeteo-->>API: Return ocean information history
API->>CLI: Return historical data
CLI->>User: Display historical data
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @AITMAR-TAFE - I've reviewed your changes - here's some feedback:
Overall Comments:
- The global
testingvariable is an anti-pattern that could cause issues in production. Consider using proper test fixtures or dependency injection instead. - There is significant code duplication between the historical and current data functions. Consider extracting common API call logic and data processing into shared utility functions.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 6 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -14,6 +15,8 @@ | |||
|
|
|||
| from src import helper | |||
|
|
|||
| testing = 1 | |||
There was a problem hiding this comment.
suggestion: Consider replacing the global testing flag with dependency injection or a configuration object
Global variables for testing make code harder to maintain and test. Consider passing this as a parameter or using a configuration object that can be injected into the functions.
class Config:
def __init__(self, testing=False):
self.testing = testing
config = Config(testing=True)
| @@ -90,6 +93,76 @@ def get_uv(lat, long, decimal, unit="imperial"): | |||
| return current_uv_index | |||
|
|
|||
|
|
|||
| def get_uv_history(lat, long, decimal, unit="imperial"): | |||
There was a problem hiding this comment.
suggestion: Extract common API setup and error handling logic into a shared utility function
There's significant duplication in API setup and error handling between get_uv_history and ocean_information_history. Consider creating a shared utility function to handle these common operations.
|
|
||
| # For testing purposes if testing equals 1 it will continue | ||
| if testing == 1: | ||
| # Attempt to fetch the UV index data from the API |
There was a problem hiding this comment.
issue (typo): Fix misleading comment in ocean_information_history
This comment incorrectly refers to UV index data when the function is actually fetching ocean data.
| responses = openmeteo.weather_api(url, params=params) | ||
|
|
||
| except ValueError: | ||
| return "No data" |
There was a problem hiding this comment.
suggestion: Implement consistent error handling across functions
Different functions return different types on error ('No data' string vs list). Consider implementing a consistent error handling strategy that maintains return type consistency.
| return "No data" | |
| return [] |
| def test_get_uv_history_basic_functionality(): | ||
| """ | ||
| Test the basic functionality of the get_uv_history function. | ||
|
|
||
| This test verifies that the function returns a string | ||
| when provided with valid latitude and longitude coordinates. | ||
| """ | ||
| uv = get_uv_history(31.9505, 115.8605, 2) # Perth coordinates | ||
| assert isinstance(uv, str) |
There was a problem hiding this comment.
suggestion (testing): Test should verify the format and range of UV values
While checking the return type is good, the test should also verify that the UV value is within the expected range (typically 0-11) and has the correct decimal precision as specified in the input parameter.
| def test_get_uv_history_basic_functionality(): | |
| """ | |
| Test the basic functionality of the get_uv_history function. | |
| This test verifies that the function returns a string | |
| when provided with valid latitude and longitude coordinates. | |
| """ | |
| uv = get_uv_history(31.9505, 115.8605, 2) # Perth coordinates | |
| assert isinstance(uv, str) | |
| def test_get_uv_history_basic_functionality(): | |
| """Test the basic functionality of the get_uv_history function.""" | |
| uv = get_uv_history(31.9505, 115.8605, 2) | |
| assert isinstance(uv, str) | |
| uv_float = float(uv) | |
| assert 0 <= uv_float <= 11 | |
| assert len(uv.split('.')[-1]) <= 2 |
tests/test_helper.py
Outdated
| def test_print_historical_data(): | ||
| """ | ||
| Tests the print_historical_data function for correct output. | ||
|
|
||
| This test verifies that the function prints the expected information | ||
| from a dictionary containing historical ocean data. It captures the | ||
| printed output and checks if specific test values are present in | ||
| the output. | ||
| """ | ||
|
|
There was a problem hiding this comment.
issue (testing): Test uses overly simplistic test data
Instead of using 'test' as the value, use realistic data that matches what would be returned from the API. This would make the test more meaningful and help catch formatting issues.
tests/test_api.py
Outdated
| @patch('src.api.testing', new=0) # Set testing variable to 0 | ||
| def test_get_uv_history_api_response(): | ||
| """ | ||
| Test how get_uv_history handles API response. | ||
|
|
||
| This test verifies that the function returns the expected values | ||
| when called with valid coordinates while patching the API call request. | ||
| """ | ||
| result = get_uv_history(31.9505, 115.8605, 1) | ||
| expected_result = '0.6' | ||
| assert result == expected_result |
There was a problem hiding this comment.
suggestion (testing): Missing test for API error handling
Add tests to verify how the function handles API timeouts, connection errors, and malformed responses. These are important edge cases for API interactions.
@patch('src.api.testing', new=0)
def test_get_uv_history_responses():
assert get_uv_history(31.9505, 115.8605, 1) == '0.6'
with patch('requests.get', side_effect=requests.Timeout):
assert get_uv_history(31.9505, 115.8605, 1) == 'Error'
with patch('requests.get', side_effect=requests.ConnectionError):
assert get_uv_history(31.9505, 115.8605, 1) == 'Error'
tests/test_helper.py
Outdated
| def test_print_historical_data_various_data_types(): | ||
| """ | ||
| Tests the print_historical_data function line by line. | ||
|
|
||
| This test verifies that the function can handle and | ||
| print various data types | ||
| (such as integers, and floats) correctly, and ensures that | ||
| each piece of data is displayed with the appropriate message. | ||
| """ | ||
|
|
||
| # Prepare test data with various data types | ||
| test_ocean_data_dict = { | ||
| "UV Index one year ago": 5.2345, | ||
| "Height one year ago": 4.1123, | ||
| "Swell Direction one year ago": 230, | ||
| "Period one year ago": 9.5 | ||
| } | ||
|
|
||
| # Capture the printed output | ||
| captured_output = io.StringIO() | ||
| sys.stdout = captured_output | ||
|
|
||
| # Call the function | ||
| helper.print_historical_data(test_ocean_data_dict) | ||
| sys.stdout = sys.__stdout__ | ||
|
|
||
| # Get the printed response | ||
| print_response = captured_output.getvalue().strip() | ||
|
|
||
| # Check if the values are correctly formatted in the output | ||
| assert "UV Index: 5.2" in print_response | ||
| assert "Wave Height: 4.1" in print_response | ||
| assert "Wave Direction: 230" in print_response | ||
| assert "Wave Period: 9.5" in print_response |
There was a problem hiding this comment.
suggestion (testing): Test should verify handling of null or missing values
Add test cases for when some historical data points are missing or null, as this is a common scenario with historical data.
def test_print_historical_data_various_data_types():
test_ocean_data_dict = {
"UV Index one year ago": 5.2345,
"Height one year ago": None,
"Swell Direction one year ago": 230,
"Period one year ago": ""
}
captured_output = io.StringIO()
sys.stdout = captured_output
helper.print_historical_data(test_ocean_data_dict)
sys.stdout = sys.__stdout__
| def test_ocean_information_history_basic_functionality(): | ||
| """ | ||
| Test the basic functionality of the ocean_information_history function. | ||
|
|
||
| This test checks that the function returns actual values for | ||
| the wave data points when provided with valid coordinates. | ||
| """ | ||
| waves = ocean_information_history(31.9505, 115.8605, 2) | ||
| assert waves[0] is not None | ||
| assert waves[1] is not None |
There was a problem hiding this comment.
issue (testing): Test assertions are too basic
The assertions should be more specific. Instead of just checking for not None, verify that the values match the expected format and ranges for wave height, direction, and period.
src/helper.py
Outdated
| @@ -173,6 +188,35 @@ def print_ocean_data(arguments_dict, ocean_data_dict): | |||
| print(f"{label}{ocean_data_dict[data_key]}") | |||
|
|
|||
|
|
|||
| def print_historical_data(ocean_data_dict): | |||
There was a problem hiding this comment.
issue (complexity): Consider consolidating historical data printing into the existing print_ocean_data() function instead of creating a separate function.
The new print_historical_data() function introduces unnecessary duplication. The existing print_ocean_data() function already has a flexible mapping system that can handle these historical data points. Consider removing print_historical_data() and extending print_ocean_data() instead:
def print_ocean_data(arguments_dict, ocean_data_dict):
# Add section printing capability
def print_section(title):
if any(arguments_dict.get(arg) == 1 for arg in ['show_past_uv', 'show_height_history',
'show_direction_history', 'show_period_history']):
print(f"\n{title}")
mappings = [
("show_uv", "UV Index", "UV index: "),
("show_height", "Height", "Wave Height: "),
("show_direction", "Swell Direction", "Wave Direction: "),
("show_period", "Period", "Wave Period: "),
# Print current data first...
]
for arg_key, data_key, label in mappings:
if int(arguments_dict[arg_key]) == 1:
print(f"{label}{ocean_data_dict[data_key]}")
# Print historical section
print_section("Historical Ocean Data (1 Year Ago):")
historical_mappings = [
("show_past_uv", "UV Index one year ago", "UV index: "),
("show_height_history", "Height one year ago", "Wave Height: "),
("show_direction_history", "Swell Direction one year ago", "Wave Direction: "),
("show_period_history", "Period one year ago", "Wave Period: ")
]
for arg_key, data_key, label in historical_mappings:
if int(arguments_dict[arg_key]) == 1:
print(f"{label}{ocean_data_dict.get(data_key, 'No data')}")This approach:
- Maintains the existing mapping pattern
- Adds section headers when needed
- Keeps all data printing logic in one place
- Makes it easier to add new historical metrics
Codecov ReportAttention: Patch coverage is
|
There was a problem hiding this comment.
Hey @AITMAR-TAFE,
Awesome work! It looks like you took your time and make a lot of good changes/additions. I like that you've implemented testing, it looks thorough.
Can you include some documentation on how to retrieve the past surf data in the readme? I'm trying to run this locally right now and it would be nice if there were some instructions on how to do this
Also, the linter failed! In your local environment, can you run make lint? It looks like there's only one error:
make lint
poetry run ruff check src tests
src/api.py:508:5: PLR0914 Too many local variables (17/15)
Found 1 error.
make: *** [makefile:21: lint] Error 1
which should be an easy fix!
I just tested the new feature on my local environment and it works great. One thing though, I was thinking this can not be shown on default, and if the user wants to see this information it can be shown (ex: curl localhost:8000?loc=perth,show_historical_data). I know this wasn't speicifed when you started the issue and you've done a lot of work, so I can also implement this if you're short on time/dont want to, haha.
Regardless, thanks for your contribution. Once we iron out some of the small stuff it'll be ready to merge 👍
|
@all-contributors please add @AITMAR-TAFE for code |
|
I've put up a pull request to add @AITMAR-TAFE! 🎉 |
…istorical data - Removed the default print function from the helper module. - Updated the argument handling to support historical data. - Added tests for historical wave period, direction, height, and UV arguments.
AITMAR-TAFE
force-pushed
the
feat/historical-data
branch
from
3e928bf to
54230b1
Compare
|
Hey @ryansurf, Thank you for your feedback! I appreciate your kind words about the changes and the testing implementation. I did consider removing the default print function but hesitated because it was specified in my original issue. However, I have now fixed the issue that caused the linter to fail and removed the print function as you suggested. I also updated the user arguments and revised the README file to include instructions on retrieving all historical data arguments. Lastly, I added some tests for historcial data arguments to ensure everything works smooth. Just let me know if there is anything else I can improve :) ! |
|
@AITMAR-TAFE just tested the changes, its working wonderfully! Just an fyi, the linter failed again. However, this time its asking for the formatter to be ran, so I'll just merge the PR and take care of it on my end. Thats on me, I should have advised you to run that as well ( Also, it would be nice to implement abbreviations for the arguments, like Regardless, great work! You've written clean, documented code with test cases. I really appreciate your efforts, thank you for the contribution Edit: Actually, I'll wait a bit to merge it. If you want to run the formatter on your end, please do! If not, I'll merge the PR and do it myself in the next day or two 🥂 |
|
Hey @ryansurf , thanks for the feedback, and apologies for the back and forth! I've now taken care of the formatter and added the suggested abbreviations for the arguments. I really appreciate you taking the time to review my work. Thanks again for the support! |
This pull request introduces a feature that enhances user experience by allowing users to compare current weather conditions with historical data from one year ago. This capability facilitates a better understanding of seasonal patterns, informs decision-making, and fosters awareness of climate trends. With versatile data presentation options through CLI prompts and a dedicated print function, users can access information in their preferred format. Comprehensive documentation and unit tests ensure clarity, maintainability, and reliability of the new functionality. Overall, this feature enriches user engagement and provides valuable insights into weather variability and its implications.
Related to issue nr #148 .
Enhancements:
Implemented a two new function get_uv_history and ocean_information_history to fetch data one year ago.
Refactored gather_data function to include historical weather data.
Refactored arguments_dictionary , set_output_values and print_ocean_data for displaying past ocean data to maintain CLI prompt options.
Created a separate print function print_historical_data for displaying historical data as default.
Documented new functions and tests for clarity and maintainability.
Testing:
Added unit tests for the new functionality to ensure accuracy. These tests also confirm the functionality works as intended and adheres to expected outcomes.
Evidence of Implementation:
Screenshots: Included screenshot demonstrate how the feature looks in action.
Summary by Sourcery
Add a feature to compare current weather conditions with historical data from one year ago, including UV index and ocean data. Implement new functions for fetching historical data and refactor existing functions to support this feature. Update documentation and add unit tests to ensure functionality and maintainability.
New Features:
Enhancements:
Documentation:
Tests: