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

Restore ability to select PWS when providing an API key #63

Merged
merged 9 commits into from Jul 10, 2019

Conversation

Projects
None yet
2 participants
@salbahra
Copy link
Member

commented Jul 9, 2019

Adds support to view and select personal weather stations (PWS) when provided an API key. This is is for PWS owners only.

This PR also updates all packages to the latest version and changes all indentation to tabs for all files (due to inconsistencies in the PR diff). Due to this, please ignore whitespace when diffing. Github has an option for this under the gear icon at the top.

This will be used in conjunction with OpenSprinkler/OpenSprinkler-Weather#70 and OpenSprinkler/OpenSprinkler-Weather#72.

@Derpthemeus
Copy link
Member

left a comment

General feedback:

  • We need to set settings.loc to the coordinates, not the PWS ID. This way we can fallback to using Dark Sky if the WUnderground API closes again.

  • We should tell users that using a PWS isn't supported by all AdjustmentMethods (namely ETo).

I also have some feedback specifically regarding main.js, but I can't directly comment on the file since the diff is too large:

  • Line 2889: What errors is this check catching? I think AJAX error should be handled by using a fail method instead of the done function, and the WUnderground API doesn't appear to use this field (and even if it did, errors should be detected by checking the response status code).

  • Line 3686: Did you intentionally say we use OWM (even though we'll use Dark Sky by the time this deployed) so we don't need to worry about updating translations, or is this an oversight? Also, we should clarify that both the API key and PWS ID need to be set for WUnderground to be used.

@salbahra

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

@Derpthemeus Feedback on the following:

  1. For the location, I believe we are using the coordinates for location here: https://github.com/OpenSprinkler/OpenSprinkler-App/pull/63/files#diff-60f1f7e7ccc2f7144af40899924f40f0R3859 and we set the wto.pws here: https://github.com/OpenSprinkler/OpenSprinkler-App/pull/63/files#diff-60f1f7e7ccc2f7144af40899924f40f0R3849.

  2. I can add that feedback in the help option once we add ETo

  3. WUnderground API returns an HTTP error but it also returns a payload which has an errors array and each error has a message and a code. The done is an alias for success so we actually need to add a fail handler instead of checking the reply for errors in success. More specifically, we don't handle a fail condition (nothing to do) so simply deleting this check should suffice. Fixed now.

  4. This was mostly because of translation reasons and because Dark Sky isn't out yet.

@salbahra

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

@Derpthemeus thank you for clarifying the first point! I have removed this input method now (typing pws:STATIONID).

@salbahra

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

For the second point, this is where I think we would add that information:
Screen Shot 2019-07-09 at 12 03 27 PM

@Derpthemeus

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

That should be perfect. After that has been added, will we need to get the message translated before we're ready to merge?

@salbahra

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

@Derpthemeus I think we should add that into the ETo PR as that's not been introduced yet, imo. Addressing localization is outside the scope of this PR but something we need to address now that getlocalization.com is offline.

@salbahra salbahra merged commit c76dd28 into master Jul 10, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@salbahra salbahra deleted the feature/restore-wunderground-for-pws branch Jul 10, 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.