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.

Sign up for GitHub

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

Jump to bottom

feat: migrate weather.lua to use WeatherAPI #448

Merged
merged 8 commits into from
Jul 6, 2024
Merged

feat: migrate weather.lua to use WeatherAPI #448

merged 8 commits into from
Jul 6, 2024

Conversation

Ryuno-Ki
Copy link
Contributor

When I looked into OpenWeatherMap 3.0 API they demanded credit card information even for their free tier. Therefore I decided to move to another provider with a generous tier.

In a first step I rewrote the current weather report (i.e. no forecast) to use it.

I haven't received any commentary on #442 so I don't know where your mind was going. If OpenWeatherMap phases out its 2.5 API and nobody else intends to use their 3.0 one, I offer to take this implementation instead.

@Ryuno-Ki
refactor: rewrite weather.lua to use WeatherAPI
39d72e4 
When I looked into OpenWeatherMap 3.0 API they demanded credit card
information even for their free tier. Therefore I decided to move to
another provider with a generous tier.

In a first step I rewrote the current weather report (i.e. no forecast)
to use it.

I haven't received any commentary on streetturtle#442 so I don't know where your
mind was going. If OpenWeatherMap phases out its 2.5 API and nobody else
intends to use their 3.0 one, I offer to take this implementation
instead.

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki Ryuno-Ki mentioned this pull request Jun 17, 2024
Open
@AlbeyAmakiir
Copy link

Really appreciated that you're doing this. :)

An error I noticed with the original implementation: If the API call did not return any JSON nor an error code (it could sometimes get html instead), the widget did not know how to handle it, and would fail to update properly.

I don't remember quite how it failed, since I edited mine to handle that (just a if string.match(stdout, "<") ~= nil then in update_widget after the first if block). I don't know if the new API will run into the same issue, but if it does, I figure now might be a good time to mention that.

@Ryuno-Ki
feat: safeguard against invalid JSON
c734c05 
It appears that OpenWeatherMap sometimes sent out bad JSON.
Learn from the lesson and do better in this PR.

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki
fix: drop unused variables
11c62fb 
Let's make Luacheck happy!

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki
fix: missed a blank line
ebcf76a 
Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki
Copy link
Contributor Author

@AlbeyAmakiir Thank you.

I've added a safeguard similar to the stderr case.
In my experience as web developer, some web services sent out HTML instead of JSON when there's an Internal Server Error (think a Reverse Proxy in front of the app server is trying to be helpful). It could have been the case here.

@streetturtle The Luacheck fails over an unused variable in the APT widget. Shall I address it in this PR?

@streetturtle
Copy link
Owner

That would be amazing! Thanks!

@streetturtle
Copy link
Owner

I'll test it today/tomorrow and will let you know. Thank you for your work!

@Ryuno-Ki
chore: remove dead code
9abce7f 
Luacheck fails over this unused code. Streetturtle gave me green light
to remove it as part of this PR.

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@streetturtle
Copy link
Owner

#442 (comment)

@Ryuno-Ki Ryuno-Ki changed the title refactor: rewrite weather.lua to use WeatherAPI WIP: refactor: rewrite weather.lua to use WeatherAPI Jun 24, 2024
@Ryuno-Ki
refactor: extract WeatherAPI widget into its own
f0cb75f 
This was done to allow for choice. In the first step I copied over the
existing code into a new folder and accompanied it with icons and
locales. There is no indicator of night in the JSON responses, which
made me delete the respective icons.

I copied and updated the README to fit the current implementation.
I will need to add screenshots before I can ask for another round of
review.

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki
refactor: restore original implementation
df72605 
This is going to be marked as deprecated by streetturtle going forward.
As of today, I am still able to successfully call the v2.5 API.

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki
chore: include screenshot of popup
d9e9ee6 
Unlike weather widget I only have the current weather in place here.
I'm okay with the default font so I have not created another screenshot.

Signed-off-by: André Jaenisch <andre.jaenisch@posteo.de>
@Ryuno-Ki Ryuno-Ki changed the title WIP: refactor: rewrite weather.lua to use WeatherAPI feat: migrate weather.lua to use WeatherAPI Jul 2, 2024
@Ryuno-Ki
Copy link
Contributor Author

Ryuno-Ki commented Jul 2, 2024

@streetturtle I feel this is ready for another round of review

@streetturtle streetturtle merged commit 09feeaa into streetturtle:master Jul 6, 2024
1 check passed
@Ryuno-Ki Ryuno-Ki deleted the weather-migration branch July 7, 2024 13:07
@vredesbyyrd
Copy link

@Ryuno-Ki

I much appreciate you taking the time to do this. WeatherAPI looks pretty nice!

Looking at the icon_map table, I'm wondering if there is anyway we can have the current weather icon distinguish between night and day? The api returns a boolean for is_day, which might be helpful? It just seems a bit jarring to see a sunny icon when it's dark outside.

I may dig into this myself, but any thoughts you might have would be very appreciated!

@Ryuno-Ki
Copy link
Contributor Author

Thank you.

Day/Night icons will be part of #455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@streetturtle streetturtle streetturtle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ryuno-Ki @AlbeyAmakiir @streetturtle @vredesbyyrd