MatthewFlamm / pynws

⛈️ A python library to asynchronously retrieve weather observation from NWS/NOAA
MIT License
33 stars 10 forks source link

Add ability to extract time data from forecast and forecast hourly #213

Closed kamiyo closed 3 weeks ago

kamiyo commented 1 month ago

Thinking ahead to home-assistant features, we should think about exposing the time properties for forecasts.

In the station I use, these are the relevant properties we can consider:

{
    "generatedAt": "2024-07-12T01:54:55+00:00",
    "updateTime": "2024-07-11T22:56:35+00:00",
    "validTimes": "2024-07-11T16:00:00+00:00/P7DT12H",
}

I suppose updateTime is the relevant one, since that is when the data was updated. But, I'm happy to implement the below logic for the other ones as well.

The way I see it working is making two new properties in SimpleNWS: _forecast_updated_time and _forecast_hourly_updated_time, which will get populated with the above updateTime within get_gridpoints_forecast() and get_gridpoints_forecast_hourly(). Then, we can add public accessors for the properties. This shouldn't break existing behavior. Let me know if this makes sense.

I'm happy to make a PR.

MatthewFlamm commented 1 month ago

I agree with this. The only question I have is what is the most relevant time, but isn't important for this package.

kamiyo commented 1 month ago

Should we expose all three time in a dict, or separately, or just the updateTime?

lymanepp commented 1 month ago

I think updateTime and validTimes would both be useful.

MatthewFlamm commented 1 month ago

I think it makes sense to keep them together in some manner. Otherwise we will end up with quite a few attributes added. What about forecast_metadata and forecast_hourly_metadata? We could have a dataclass, or a simple dict is fine since that is how all the others are returned.

kamiyo commented 1 month ago

A wrinkle - in SimpleNWS, we only have access to the extracted forecast without time metadata. In order for us to store it, we have to create properties in the parent NWS class. It seems to me that you didn't want much state in that parent class.

The two solutions are to just put the timestamp states in the parent NWS class, or to create internal functions in NWS that will get called in SimpleNWS like _get_gridpoints_forecast_with_timestamp() in order to not break existing API.

MatthewFlamm commented 1 month ago

I'm in favor of making a breaking change to the NWS class to return all the information, e.g. return periods, metadata, ....

Added: The mental model I had when constructing this was that NWS class has logic that enabled users to not have to directly worry about the flow of information needed in the API. For example, inputting a lat/lon should enable the user to get a forecast without explicitly needing to worry about the gridpoints. This class has state, but only related to the setup of the gridpoints, stations, etc. This is typically only done once. It was an oversight that we do not return all the data from the final endpoints in this class.

MatthewFlamm commented 3 weeks ago

I think this was closed in #214 . Feel free to request it to be reopened if it wasn't.