Conversation
chrisdedman
commented
Apr 13, 2026
chrisdedman
commented
- Added weather extension (ported from Grace Bot and adapted for Ada Bot).
- Required to get our own ADA_OPENWEATHER_API_KEY var env.
- Also ran black formatting locally on the codebase.
| extension = Extension("weather") | ||
|
|
||
|
|
||
| def _get_api_key() -> str | None: |
There was a problem hiding this comment.
info: The reason I left this one here instead of moving into the service module is that this depends on extension, and I didn't want to move the extension into the service module, nor import the extension into the service module.
There was a problem hiding this comment.
Totally, this definitely belong here.
| visibility: int | ||
|
|
||
|
|
||
| def _fetch_weather( |
There was a problem hiding this comment.
nit: I think api_key should come first. It's not a big deal but I think it make more sense and if we add more entry point it would be more consistent to have the api_key then the parameters for that function.
| def _fetch_weather( | ||
| city: str, | ||
| api_key: str, | ||
| session: requests.Session | None = None, |
There was a problem hiding this comment.
In what case will this be used? Do we really need that in our case?
There was a problem hiding this comment.
I added because you had it on your suggestion change. I don't think we need it, but I thought that you saw a use case for it so left it in my implementation.
| return kelvin * 1.8 - 459.67 | ||
|
|
||
|
|
||
| def _normalize_city_name(city_parts: tuple[str, ...]) -> str: |
There was a problem hiding this comment.
This should probably be in the weather_extension since it's business logic related to our command parameters.
| return WeatherError.UNAVAILABLE | ||
|
|
||
|
|
||
| def _format_weather(city: str, data: WeatherPayload) -> str: |
There was a problem hiding this comment.
It would make sense to have the formatting in the service if the WeatherPayload would returned a formatted weather payload. Usually the service won't do too much formatting; or at least still return raw values.
In our case it's part of the business logic of the extension so it probably belong in the extension not in the service.
Note: Those formatting function could be in a helper file (and be public) although I'm fine with them being directly in the extension.
| from matrix import Context, Extension | ||
| from .openweather_service import ( | ||
| WeatherError, | ||
| _normalize_city_name, |
There was a problem hiding this comment.
You're importing private function. This probably mean that their either called at the wrong place or live in the wrong place.
Imo, _normalize_city_name and _format_weather should live in the extension and stay protected. _fetch_weather should be public.
| visibility: int | ||
|
|
||
|
|
||
| def _fetch_weather( |
There was a problem hiding this comment.
This function should be public -> fetch_weather:
def fetch_weather(api_key: str, city_name: str) -> WeatherPayload | WeatherError:
...2 participants
