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

Add current weather info #71

Merged
merged 3 commits into from
Mar 17, 2021
Merged

Add current weather info #71

merged 3 commits into from
Mar 17, 2021

Conversation

rashil2000
Copy link
Member

No description provided.

@jcwillox
Copy link
Member

Looking good, is there any reason you didn't use their formatting API, in my testing I found it was actually 2x faster, then retrieving the full JSON response.

(Invoke-RestMethod wttr.in/?format="%t+-+%C+(%l)").TrimStart("+")

I know you know this, but as a general PSA we should be careful switching to/using http, while in this case, we're not transmitting any location data / identifiable information so we don't necessarily need https, but if we did start using location information from the computer we would need to switch this to https.

@rashil2000
Copy link
Member Author

rashil2000 commented Mar 15, 2021

The formatting API looks good, however, there is a subtle difference in location preciseness. My PR shows local area and region, whereas the command you mentioned shows region and country. This obviously allows for closer location-pointing. However, if the speed difference is really huge, we can sacrifice on that precision.
[For the sake of completeness, the Locale function already shows the language and the country; that's why I went with region and area name to avoid redundancy.]

As for http, most sites usually have a redirect setup, to the corresponding https URI, but as I see in this case, while ipconfig.me redirects it, wttr.in does not. It's really only syntactic convenience at this point.

I'm open to suggestions, regarding both.

@jcwillox
Copy link
Member

Yeah, I noticed that location thing too, it's up to you whether you want to use the JSON way or the formatting API, they both seem to produce the same weather info at least, however, I did notice about 630ms improvement with the formatting API.

@rashil2000
Copy link
Member Author

I ended up favoring speed instead of precision.

Thanks for the suggestion!

@rashil2000 rashil2000 merged commit 03367b5 into master Mar 17, 2021
@rashil2000 rashil2000 deleted the weather-info branch March 17, 2021 19:20
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