search

polybar / polybar-scripts

This is a community project. We write and collect scripts for polybar!
The Unlicense
2.48k stars 340 forks source link

Merging the openweathermap-* modules into a single configurable module #92

Closed dkasak closed 6 years ago

dkasak commented 6 years ago

I just made some improvements to the openweathermap-* modules and it was more tedious than necessary because they are four separate modules. As it is now, the modules share most of the functionality, with the primary difference being in what gets output in the end.

Perhaps it would make sense to merge all of these into a single module and add one or two additional configuration variables so they can share the code?

x70b1 commented 6 years ago

We had this idea already here: PR https://github.com/x70b1/polybar-scripts/pull/8 I closed this as wontfix.

I'm still of the same mind. I don't want to overengineer simple and working scripts.

But you have some good improvements in it. HTTPs requests for example.

dkasak commented 6 years ago

Ah, sorry, I forgot to search closed issues.

I do still think merging them would be better in this instance, but I understand your position. Closing.

x70b1 commented 6 years ago

Thanks!

Would you like to open a PR for your HTTPs changes? Otherwise i would do this.

dkasak commented 6 years ago

@x70b1 Do you mean a separate PR which doesn't include the city-by-name change?

x70b1 commented 6 years ago

The idea is not bad. But sometimes it leads to a wrong city. If you use the ID you are very exact.

If you encourage users to take the ID you can prevent someone from opening a bug: "Help! The script shows the wrong city?!"

Get out the ID of your home is not so hard.

dkasak commented 6 years ago

Of course, it might lead to the wrong city in some rare situations, but I think the chances of that are minimal if the user also specified the country code.

The reason I added it is that I travel a lot, so I tend to change the location a lot (Mozilla's location service doesn't give very accurate results as currently implemented, sometimes completely missing the city by ~250 km). In this situation, using IDs is a bit unwieldy... Of course, I could populate my script with several entries, such as

CITY=<id1>  # <CITY1>
CITY=<id2>  # <CITY2>
CITY=<id3>  # <CITY3>

and then comment/uncomment all but one of those based on my location. However, I thought specifying the city by name was a nice addition, particularly because it is so easily implementable.

But again, if you're not interested in the feature, don't feel pressured. I'll probably just commit a modified version of the script to a private repo.

I'll make a separate PR for the HTTPS API.