core icon indicating copy to clipboard operation
core copied to clipboard

Disable AEMET legacy options

Open Noltari opened this issue 1 year ago • 4 comments

Breaking change

The daily data for the current day wasn't available after midday and now it will be. Automations relying on day[0] for checking next day forecast will have to be reworked to use day[1].

Proposed change

This enables proper timezone handling:

  • Atlantic/Canary for the Canary Islands.
  • Europe/Madrid for the Iberian Peninsula.

Also provides the daily data for the current day after AEMET stops providing the full day interval, which is normally after midday (12:00). What the integration library does to workaround this is to fallback to the 12-24 interval data if the the 00-24 is no longer provided by the API.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [x] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Noltari avatar Jan 11 '24 11:01 Noltari

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Feb 10 '24 09:02 home-assistant[bot]

The PR looks straightforward but I don't understand why so many changes were needed to the tests.

This makes it harder to see if/how the change is being tested.

@davet2001 you're totally right, but there's nothing I can do about it but explaining the reason for that in the PR description, which I already did...

There are two different things: one is the timezone, which wasn't provided until now, and was causing a slight deviation with the hourly data, but was probably unnoticed due to Spain being GMT+1 or GMT+2, and therefore having only 1h or 2h of deviation. In the Canary Islands, on the other hand, it only had 0h or 1h of deviation.

The other change deals with the fact that the full daily data was only provided by the API up until midday. After that, the API only provides a second half day data, which is now used so that the daily data still has info about the current day from 12:00 to 24:00.

However, I'm open to suggestions on how to handle these changes.

Thanks for the review though!

Noltari avatar Feb 10 '24 10:02 Noltari

Ok, so if my understanding is correct:

  • A new mode of accessing the API returns timestamps with a timezone, therefore the .ambr test file has every timestamp changed.

  • Those timestamps now refer to a different time of day, therefore the expected values are now different.

  • You've updated both in this PR to capture this change.

I couldn't see explicitly where the timezone boundary conditions were being tested. This could be testing marginally before or after a transition time to verify it's behaving as you expect. But it could be that I missed it or you had another way of confirming this.

If the above is correct, I think the PR is ok.

davet2001 avatar Feb 10 '24 13:02 davet2001

Ok, so if my understanding is correct:

  • A new mode of accessing the API returns timestamps with a timezone, therefore the .ambr test file has every timestamp changed.
  • Those timestamps now refer to a different time of day, therefore the expected values are now different.
  • You've updated both in this PR to capture this change.

I couldn't see explicitly where the timezone boundary conditions were being tested. This could be testing marginally before or after a transition time to verify it's behaving as you expect. But it could be that I missed it or you had another way of confirming this.

I've validated every value with the changes of this PR against the official AEMET website: https://www.aemet.es/es/eltiempo/prediccion/municipios/getafe-id28065

The funny thing about that website and the reason why I didn't catch the deviation before is because the data used there isn't the fresh data from the AEMET OpenData API. The data seems to be cached and updated every X minutes, so the values from the website doesn't always match the values from the API... But sometimes, if there haven't been any changes to the data provided by the API for a couple of hours, the data will match and that's how I could confirm that now it's correct.

If the above is correct, I think the PR is ok.

Yes, thanks @davet2001 :)

Noltari avatar Feb 10 '24 14:02 Noltari

So while I understand and think it's a good change those timestamps in the forecasts should still be utc so more work is needed to get them back to it. See dev docs

Ok, I'll change it to use UTC. However, wouldn't it be easier for humans to see those timestamps in local timezone rather than UTC? This rule doesn't make much sense to me...

Noltari avatar Feb 18 '24 19:02 Noltari

@gjohansson-ST after changing the integration library, this PR now uses UTC timestamps as suggested, so we should be good to go.

Noltari avatar Feb 21 '24 15:02 Noltari

Have you tested so it looks good in the frontend? Just want to check as the tests comes back as 23H and not 00H so want to make sure the forecasts are represented on the correct day in frontend (I assume yourself are +1H hence UTC is -1H from what you actually want).

gjohansson-ST avatar Feb 22 '24 11:02 gjohansson-ST

Have you tested so it looks good in the frontend?

@gjohansson-ST yes, I've tested it on the frontend: image image image

Just want to check as the tests comes back as 23H and not 00H so want to make sure the forecasts are represented on the correct day in frontend (I assume yourself are +1H hence UTC is -1H from what you actually want).

Yes, that's expected since 00:00 GMT+1 is 23:00 UTC (or GMT+0). Spain is GMT+1 in winter and GMT+2 in summer. But right now Spain is GMT+1 and the same for the tests.

Noltari avatar Feb 22 '24 11:02 Noltari