Closed nut-3 closed 7 months ago
bjesus
commented
8 months ago hey @nut-3 - many great fixes here, thank you. I also wrote on another PR - what's the benefit of having the process always running vs spawning it every 30 minutes? I feel like if it's always running there will always be the risk of introducing some memory leak
nut-3
commented
8 months ago hey @nut-3 - many great fixes here, thank you. I also wrote on another PR - what's the benefit of having the process always running vs spawning it every 30 minutes? I feel like if it's always running there will always be the risk of introducing some memory leak
Hi, @bjesus . As for daemon vs spawning. Most important draw is the ability to limit wttr.in requests frequency. With spawning you can only rely on users' decency to not send requests too often. Another one is that it will be easier to sync daemon instances' data in multi-monitor setup. As you may know, waybar spawns one custom module process for each monitor on which this custom module is configured. I've thought that in the future it might be interesting to implement feature that lets several wttrbar processes to use common response data from wttr.in. Well, If you don't like such drastic architectural changes, feel free to just copy parts you want. Or I can make another pr with parts you want in you project.
bjesus
commented
8 months ago Hey, thanks for clarifying. I wouldn't want to hurt the wttr.in service but I also see limiting the requests frequency as a out of the scope here - I'll happily add a notice to the readme though. I'm more concerned about breaking the current behavior of wttrbar for existing users who will just update and expect things to work as usual.
all the rest of the PR seems great and I'd love to merge it. If you can make a PR without the constant running process I would to merge it, otherwise I'll try looking into it myself sometime next week 🙏
dianaw353
commented
8 months ago I think we need to use nut-3 method as wttr.in been having stuggles being online this week as the server been over max amount of request... If you still want to have the old method can we have a flag for those that want to use the old method of getting requests and have the new method be the default so save the wttr.in servers
bjesus
commented
8 months ago (1) I saw no indication that wttr.in was down because of load, and I would be very flattered to think that if it was, wttrbar has anything to do with it. (2) afaict, the wttr.in repo itself includes examples (e.g. tmux) that are querying every minute. (3) changing the way wttrbar works, from what I understand, can actually cause more traffic to wttr.in, because most users will not update their waybar config, meaning that new processes will be spawned and each process will keep sending a request every given interval.
I released 0.7.1 now which includes a simple 10 minutes caching. It simply saves the response to a temporary JSON file, and if the file is fresh (i.e. newer than 10 minutes ago) it just reads from it instead of making a request. This should work seamlessly to all existing users, and would make sure we never have to worry about memory leaks as the process still dies after execution.
dianaw353
commented
8 months ago Nice I like the fact it's caching the response now instead of sending a request to get info every second. I would be Oki merging nut-3 and my fork with that change
bjesus
commented
7 months ago closing this PR as it has conflicts and doesn't seem to be updated. please open a new merge-able if you want
Changes:
--indicatorwith templates instead of--main-indicator. Example in README.md--imperialoption instead of--fahrenheit. It now makes wind speed to be displayed in MPH.--fahrenheitis a hidden synonym for now.--intervalto control it (in minutes, minimum is 30). Added corresponding changes into README.md. Note: if you have wttrbar module on more then one output, it will spawn same number of processes and they will fetch data from wttr.in independently. Prior version. that usedintervalsetting of waybar module suffers from this too, but wttrbar process is not running constantly, so it is not noticeable."interval"field now.This PR includes changes from #22, so it may be merged after or instead of it.