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 support for the Dark Sky API #24

Merged
merged 8 commits into from May 13, 2019

Conversation

Projects
None yet
3 participants
@Derpthemeus
Copy link
Member

commented May 13, 2019

The weather provider can be selected by setting the environment variable WEATHER_PROVIDER to either OWM (the default) or DarkSky. If using the Dark Sky API as the provider, the environment variable DARKSKY_API_KEY must be set.

@salbahra

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Can we update the README with the environment variable data provided in this description please?

server.ts Outdated
@@ -42,13 +46,10 @@ app.use( function( req, res ) {
} );

// Start listening on the service port
app.listen( port, host, function() {
app.listen( +port, host, function() {

This comment has been minimized.

Copy link
@salbahra

salbahra May 13, 2019

Member

What does this plus sign indicate? Thanks!

This comment has been minimized.

Copy link
@Derpthemeus

Derpthemeus May 13, 2019

Author Member

It's just casting the string to a number, I can also change it to parseInt(port) if you think that's more readable.

This line was changed in that commit because changing the way modules were imported provided the TypeScript transpiler with type information for app.listen, revealing that it expects that parameter to be a number.

This comment has been minimized.

Copy link
@salbahra

salbahra May 13, 2019

Member

Ah okay, yes please I think thats more readable. I didn't realize we could apply that in the arguments, neat!

This comment has been minimized.

Copy link
@Derpthemeus

Derpthemeus May 13, 2019

Author Member

It would actually need to be changed to typeof port === "number" ? port : parseInt ( port ) since port can be either a string or a number, and parseInt only accepts a string. Should I still change it, or is that even harder to read?

This comment has been minimized.

Copy link
@Derpthemeus

Derpthemeus May 13, 2019

Author Member

Disregard that, I can just use parseInt on line 11 to make it cleaner

This comment has been minimized.

Copy link
@salbahra

salbahra May 13, 2019

Member

Good point, just leave it as a plus. We can put a comment port is being typecasted with the plus if we want.

Derpthemeus added some commits May 13, 2019

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

I thought DarkSky T&Cs prohibited this.

See here.

But I hope I'm wrong.

@Derpthemeus

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

@PeteBa Good point, I actually hadn't read over the ToS (oops). It looks like there are 2 possible issues here:

  1. We need to add attribution to Dark Sky. We'll make sure we add this to the mobile app and controller web UI before using Dark Sky as a weather provider, but we might merge this before that happens so we don't have to worry about rebasing later.
  2. It might look like we're asking end-users to get their own API keys, but the only people we're requiring to do this are people who want to run the weather server locally. End-users don't need to provide their own key if they're using the official OpenSprinkler weather server.

Do these points address your concerns, or was there another issue you were worried about?

@salbahra salbahra merged commit 57e726d into OpenSprinkler:master May 13, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Hmm, I thought the point of going OWM was because the cost to OS of WU/DarkSky/OWM History was prohibitive. If that is not the case then why use OWM as the default. Surely, DarkSky is a better choice for the default provider.

Also, just seen that you are using forecast data from DarkSky for water level. I am curious why you wouldn't go with history given the price is the same?

I may have missed something as reading fast 8)

@Derpthemeus

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Great point! I wasn't too familiar with how the Zimmerman algorithm worked, so I setup the Dark Sky WeatherProvider to return data in an identical format to the one that was already used by the OWM WeatherProvider. I'll update it to use historical data instead of forecasted data.

Thanks for the feedback!

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Can I just double-check, the intent of this PR is to allow end-user to select DarkSky historical data when using the cloud based OS Weather Service ? If that is the case then: 1) what happened to the cost problem, and 2) why retain OWM forecasts ?

Also, this is the quote from the DarkSky T&Cs:
Your permission to use the Service is condition upon your agreement that you will not: ... direct your end-users to sign up for their own API account (The Dark Sky API not intended to be used directly by end-users);

@salbahra

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Just to add some clarification on the intended path: The cost problem has not been solved however we understand some users maybe running the weather service locally so we are adding support for it now even though we are not ready to go live with it on production (weather.opensprinkler.com). Right now we are using this a test bed for multi-provider support.

OWM is being kept as it's the solution being used for now until the above (cost) problem is solved.

That's the plan right now :) I do not intend to ever ask user's to make their own key for DarkSky as we don't want to break T&C's.

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Hey, I get it. I think multiple providers is the way to go. DarkSky is a great source for historical, accurate micro-climate data. As much as I admire OWM, I didnt add the pws endpoint for nothing 8)

Suggest you revert Step 4b in the instructions here.

@salbahra

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Completely see where you are coming from however the reason I want to keep instruction 4b is because I see that targeting developers not end users. If we want to be abundantly clear we could clarify this to read something along the lines of: For local development, please register for your own key here....

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Hmm, I read "end-users" as meaning customers which everyone using a local Weather Service would be. Having said that, I very much doubt OS is going to generate a massive useage spike for DarkSky! Rather than a developer note on Step 4, I would change the Heading Title to something like "Creating a Development Environment on a Raspberry Pi". Makes the intent clear.

@salbahra

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I consider people running the service themselves to be their own entity. The only one we can speak for is our hosted solution on weather.opensprinkler.com. Otherwise, we don't use or expose any of Dark Sky's API and therefore are not subject to any of their T&C's.

@PeteBa

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Yeah, I think that is the way Home Assistant (home automation software) gets away with their users registering their own API key and having DarkSky widgets on their home assistant dashboards. Home Assistant doesn't have an API key so isn't subject to their T&Cs.

Anyway, I really like what @Derpthemeus has done here (both the ts stuff and the multi-service support). It's great stuff!

@Derpthemeus Derpthemeus deleted the Derpthemeus:darksky branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.