Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to the v3.0 weather API #205

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Update to the v3.0 weather API #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Guppi.Console/Guppi.Console.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<PackageProjectUrl>https://github.com/rprouse/guppi</PackageProjectUrl>
<RepositoryUrl>https://github.com/rprouse/guppi</RepositoryUrl>
<PackageId>dotnet-guppi</PackageId>
<Version>6.3.2</Version>
<Version>6.3.3</Version>
<PackAsTool>true</PackAsTool>
<ToolCommandName>guppi</ToolCommandName>
<PackageOutputPath>./nupkg</PackageOutputPath>
Expand Down
2 changes: 1 addition & 1 deletion Guppi.Console/Properties/launchSettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"profiles": {
"Guppi.Console": {
"commandName": "Project",
"commandLineArgs": "todo sync"
"commandLineArgs": "weather daily"
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

The "weather daily" command is properly implemented but needs documentation updates

The command is properly implemented in WeatherSkill.cs with a dedicated handler:

  • daily command is registered with description "Views the daily weather"
  • It has a proper handler method Daily(location)

However, the documentation needs to be updated to include this command option. The current documentation only mentions basic weather functionality without detailing the available subcommands (view/now, hourly, daily).

Action items:

  • Add documentation for the "daily" subcommand in the weather section of the documentation
  • Include examples of usage with the --location parameter
🔗 Analysis chain

Verify the new command and consider updating documentation.

Please ensure that:

  1. The "weather daily" command is properly implemented and tested with the new v3.0 API
  2. The change in default debug configuration is communicated to the development team

Let's verify if this command is properly documented:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the command is documented in README or help files
rg -i "weather daily" --type md

Length of output: 33

Script:

#!/bin/bash
# Let's check for weather command implementations and documentation more broadly
# First, look for weather-related command handlers or implementations
ast-grep --pattern 'class $_ { 
  $$$ 
  Weather$_($$$) { 
    $$$ 
  } 
  $$$ 
}'

# Also search for any weather-related command registrations
rg -i "command.*weather" -A 3

# Check for any documentation files that might contain weather command info
fd -e md -e txt -x cat {} \; | rg -i "weather"

Length of output: 2588

}
}
}
2 changes: 1 addition & 1 deletion Guppi.Core/Services/WeatherService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public async Task<WeatherForecast> GetWeather(string latitude, string longitude)
throw new UnconfiguredException("Please configure the weather provider");
}

var weather = await _restService.GetData<WeatherResponse>($"https://api.openweathermap.org/data/2.5/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}");
var weather = await _restService.GetData<WeatherResponse>($"https://api.openweathermap.org/data/3.0/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}");
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider utilizing new API features

The v3.0 API offers additional features not currently used:

  • Hyperlocal minute forecasts
  • Extended daily forecasts
  • Government weather alerts
  • Historical weather data

Consider extending the WeatherResponse model and API to support these features. Example parameters:

-$"https://api.openweathermap.org/data/3.0/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}"
+$"https://api.openweathermap.org/data/3.0/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}&minutely=1&alerts=1"

Committable suggestion was skipped due to low confidence.

🛠️ Refactor suggestion

Add API version to configuration

Consider making the API version configurable to facilitate future migrations and testing.

Extract the API version to configuration:

+private const string DEFAULT_API_VERSION = "3.0";
+
 var weather = await _restService.GetData<WeatherResponse>(
-    $"https://api.openweathermap.org/data/3.0/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}"
+    $"https://api.openweathermap.org/data/{_configuration.ApiVersion ?? DEFAULT_API_VERSION}/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}"
 );

Committable suggestion was skipped due to low confidence.

⚠️ Potential issue

Enhance error handling for v3.0 specific scenarios

The current error handling only checks for configuration. Consider adding handling for:

  • Rate limit exceeded responses
  • New error response formats in v3.0
  • API subscription status

Add specific exception handling:

 var weather = await _restService.GetData<WeatherResponse>($"https://api.openweathermap.org/data/3.0/onecall?lat={latitude}&lon={longitude}&appid={_configuration.ApiKey}");
+if (weather == null)
+{
+    throw new WeatherApiException("Failed to retrieve weather data");
+}
+if (weather.IsRateLimitExceeded)
+{
+    throw new RateLimitExceededException("Daily API call limit exceeded");
+}
 return weather.GetWeather();

Committable suggestion was skipped due to low confidence.

return weather.GetWeather();
}

Expand Down
Loading