Closed EdwinB12 closed 8 months ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Attention: 18 lines
in your changes are missing coverage. Please review.
Comparison is base (
8ccc262
) 89.12% compared to head (8a21db1
) 89.16%.:exclamation: Current head 8a21db1 differs from pull request most recent head 14e22ca. Consider uploading reports for the commit 14e22ca to get more accurate results
Files | Patch % | Lines |
---|---|---|
dtbase/ingress/ingress_weather.py | 83.15% | 16 Missing :warning: |
dtbase/backend/api/sensor/routes.py | 50.00% | 1 Missing :warning: |
dtbase/core/utils.py | 87.50% | 1 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
TODO but perhaps different PR: Azure Functions Utility. Hopefully can be included into BaseIngress
Agreed, this can be a separate PR.
@mhauru - I'm reading in some hardcoded responses for mocking purposes (such as MOCKED_CONST_OPENWEATHERMAP_HISTORICAL_URL_RESPONSE). They are quite large dictionaries. Have you got any strong opinions on where I should store these dictionaries:
The separate resource files sound good to me.
One thing I was thinking of was that maybe there should be a separate pair of OpenWeatherMap sensors (historical and forecast) for every lat/long. That the lat and the long would be in the unique identifier of the sensor. Maybe this is for a separate PR though. I'm in general not fully happy with how we currently use environment variables, there may be a bit of an overhaul to do there, but it might make sense to do once things are running on Azure, since envvars are often the best way to handle parameters there.
I guess we assume for now that a single entity would only use one lat and long for something liek weather. But agreed, I think it should be included in the sensor table.
Includes:
TODO before merging :
get_data
method.TODO but perhaps different PR: