Skip to content

Conversation

HugoLiconV
Copy link
Contributor

It uses wttr.in to get the weather information and the current city based on the IP address

Solves issue #16

It uses [wttr.in](https://github.com/chubin/wttr.in) to get the weather information and the current city based on the IP address
@ethancedwards8
Copy link
Member

Nice, it's working for me

Copy link
Member

@danerwilliams danerwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great. Only thing is I think we should keep it consistent with how it looked before.
So I think the only differences I see are:

  1. Instead of the normal unicode characters for forecast it is showing the emoji, it should just be the all black colored one so that it is easy to see with the orange background and matches the other characters. See the forecast_unicode() code block from the original weather.sh script
  2. No plus for positive degrees, only the minus for negative
  3. Instead of city, country, could we do city, region? We can get the region using ipinfo.io/region

@ethancedwards8
Copy link
Member

Is ipinfo a global api? Also just my two cents, I think we should try and avoid using two external api’s

Show unicode characters for forecast instead of an emoji.
Remove the plus sign for positive degrees.
Show city, region instead of city, country
@ethancedwards8
Copy link
Member

@HugoLiconV what's the status on this? I would like to get this merged for you but it needs to be fully working :)

Also, minor issue that we can solve with #42 is that currently the CPU and Weather module have a color conflict. Nothing wrong on your part we just need something to fix #42 :)

@HugoLiconV
Copy link
Contributor Author

for

Hi @ethancedwards8, the code should be working now, I did some changes but I didn't receive a response or review from @danerwilliams

@danerwilliams
Copy link
Member

@HugoLiconV sorry I think I have changes requested still so that is why it hadn't been merged

@HugoLiconV
Copy link
Contributor Author

@HugoLiconV sorry I think I have changes requested still so that is why it hadn't been merged

I updated the PR after you requested the changes, could you tell me what it's missing?

@danerwilliams
Copy link
Member

Oh I'm not sure why but the change request didn't resolve. Either @ethancedwards8 or I need to test and confirm the change but otherwise it seems fine.

@ethancedwards8
Copy link
Member

It's working well for me. Since this is such a big change I think @danerwilliams should also test it out, then we can merge it if there's no issues. Just in case.

@ethancedwards8
Copy link
Member

I've been using it the past 2 days and it's worked great. Merging! :)

@sheikki
Copy link

sheikki commented Nov 17, 2020

In general, I use tmux on VPN'd connections on remote servers/clusters. As such, the IP-tied weather info isn't very relevant to me. I would like it to be though..

@kedare
Copy link

kedare commented Sep 15, 2021

Is there a way to force the location ? The IP addressing plan of my ISP is not location based but national-wide so I get the weather from Paris instead of my city.

@ypicard
Copy link

ypicard commented Oct 13, 2021

I agree, forcing the location is needed when using a VPN.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants