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.

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

Bug fix: UV location must use district globalId #13

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Bug fix: UV location must use district globalId #13

merged 2 commits into from
Sep 13, 2023

Conversation

tokenize47
Copy link
Contributor

@tokenize47 tokenize47 commented Sep 12, 2023

The current uv index retrieval implementation does not work properly due to wrong global id being used. This will result in an unknown result most of the times. To fix this, the closest district id must be used instead of the current location.

After this is fixed, I already have a PR ready to go on Home Assistant to include the UV Index as a sensor.

@tokenize47
Copy link
Contributor Author

tokenize47 commented Sep 12, 2023

I also made some changes to the tests because they were not working for me. From what I gathered, the fixtures folder path was wrong, but feel free to change it in case the issue was specific with my setup.

There was also a test dependency missed, which I added to the requirements file.

@dgomes
Copy link
Owner

dgomes commented Sep 12, 2023

Acho que podemos falar em português :)

UV index nunca cheguei a terminar... estava mesmo era uma copia do risco de incendio. Obrigado :)

Agora o path das fixtures é que me está a fazer espécie... partir de onde executas os testes ?

@tokenize47
Copy link
Contributor Author

Tens toda a razão.. Estava a executar diretamente do ficheiro, por isso é que não funcionava.
Já reverti essa parte.

@dgomes
Copy link
Owner

dgomes commented Sep 12, 2023

Os distritos não mudam, melhor fazer o pedido no get() e guardar como cache.

@tokenize47
Copy link
Contributor Author

tokenize47 commented Sep 13, 2023 via email

@dgomes dgomes merged commit 87ddaeb into dgomes:master Sep 13, 2023
1 check passed
@dgomes
Copy link
Owner

dgomes commented Sep 13, 2023

Obrigado!

@tokenize47
Copy link
Contributor Author

Ora essa, o prazer é meu!

Já agora, consegues dar bump na depedencia no Home Assistant que eu depois abro logo o PR para adicionar o UV Index como sensor? Visto seres o autor da integração devem aceitar o teu PR mais rápido!

@dgomes
Copy link
Owner

dgomes commented Sep 13, 2023

Abre tu o bump com o sensor extra, e eu posso fazer o review ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants