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 NOAA tide information (new PR) #15947

Merged
merged 8 commits into from Aug 22, 2018

Conversation

@jcconnell
Contributor

jcconnell commented Aug 12, 2018

Description:

This sensor adds support for NOAA Tide information. This is a new PR as requested in 15664

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5881

Example entry for configuration.yaml (if applicable):

senor:
  - platform: noaa_tides
    station_id: 8721164

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.
.coveragerc Outdated
@@ -684,6 +684,7 @@ omit =
homeassistant/components/sensor/nederlandse_spoorwegen.py
homeassistant/components/sensor/netdata.py
homeassistant/components/sensor/neurio_energy.py
homeassistant/components/sensor/noaa-tides.py

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 13, 2018

Member

Typo in the module name.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Aug 13, 2018

I think the pylint job is timing out due to the install of pandas adds significant time and puts us above 30 min. We need to bump the 30 min to maybe 40 min.

@jcconnell

This comment has been minimized.

Contributor

jcconnell commented Aug 13, 2018

Thank you for taking another look. Sorry for the typo. I copied it directly from the previous repo where I had already fixed that.

@MartinHjelmare MartinHjelmare referenced this pull request Aug 13, 2018

Closed

Increase travis tox timeout to 40 min #15960

0 of 2 tasks complete
CONF_STA_ID = 'station_id'
CONF_TZ = 'timezone'
CONF_UNITS = 'units'

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

We have CONF_UNIT_SYSTEM in const.py. Please use that.

_LOGGER = logging.getLogger(__name__)
CONF_STA_ID = 'station_id'

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

Should be CONF_STATION_ID = 'station_id'.

_LOGGER = logging.getLogger(__name__)
CONF_STA_ID = 'station_id'
CONF_TZ = 'timezone'

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

Should be CONF_TIMEZONE = 'timezone'.

@@ -0,0 +1,128 @@
"""
This component provides HA sensor support for the NOAA Tides and Currents API.

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

It's a platform.

CONF_UNITS = 'units'
DEFAULT_ATTRIBUTION = "Data provided by NOAA"
DEFAULT_NAME = 'NOAA Tides'
DEFAULT_TZ = 'lst_ldt'

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

DEFAULT_TIMEZONE

SCAN_INTERVAL = timedelta(minutes=60)
TZS = ['gmt', 'lst', 'lst_ldt']
UNITS = ['english', 'metric']

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

It's the unit system, thus UNIT_SYSTEMS.

@property
def state(self):
"""Return the state of the device."""
if self.data is not None:

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

Change this to a guard cause. Fail as early as possible.

else:
units = UNITS[0]
add_devices([NOAATidesAndCurrentsSensor(name, sta, t_z, units)], True)

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

You should check during the platform setup if the given station is available. If not then the platform setup should fail. Otherwise the users end-up with a non-functional platform.

if self.data is not None:
if self.data['hi_lo'][1] == 'H':
attr['high_tide_time'] = \
self.data.index[1].strftime('%I:%M %p')

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

The output format for all timestamps should be ISO 8601.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 14, 2018

Member

I also thought the same way, but then I realized that this is a US service only. But ISO 8601 would be nice though.

return "High tide at {}".format(tidetime)
if self.data['hi_lo'][0] == 'L':
tidetime = api_time.strftime('%I:%M %p')
return "Low tide at {}".format(tidetime)

This comment has been minimized.

@fabaff

fabaff Aug 14, 2018

Member

The current implementation of this sensor feels like a binary sensor. There is either high or low.

I guess that the same would apply for the worldtides sensor or was discussed there to make it a sensor platform.

@@ -739,6 +739,9 @@ pyTibber==0.4.1
# homeassistant.components.switch.dlink
pyW215==0.6.0
# homeassistant.components.sensor.noaa_tides
py_noaa==0.2.2

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 14, 2018

Member

Please comment this by adding it to the COMMENT_REQUIREMENTS list in script/gen_requirements.py. Then run the script again. This will avoid installing the library in our travis jobs, and should also avoid the timeout.

@jcconnell

This comment has been minimized.

Contributor

jcconnell commented Aug 15, 2018

@fabaff thank you for all the details. I'm still learning! I made all the changes you requested, plus some extras that followed in line with your requests. I also changed all timestamps to ISO8601 format except the timestamp in the state. I left this in a more human-readable form. Looking at the ISO8601 timestamp just wasn't as visually appealing or easy to determine the hour of the next hi/low tide. Would you consider this acceptable?

@jcconnell

This comment has been minimized.

Contributor

jcconnell commented Aug 15, 2018

From TravisCI on the Python 3.5.3 Pylint command:

homeassistant/components/sensor/noaa_tides.py:115:8: E0401: Unable to import 'py_noaa' (import-error)

def update(self):
"""Get the latest data from NOAA Tides and Currents API."""
from py_noaa import coops

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 16, 2018

Member

Disable the pylint warning (import-error) here.

timezone, unit_system)
noaa_sensor.update()
if noaa_sensor.data is None:
return

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 16, 2018

Member

Log an error here so the user knows why no sensor was added.

def update(self):
"""Get the latest data from NOAA Tides and Currents API."""
from py_noaa import coops # pylint: disable=import-error

This comment has been minimized.

@houndci-bot

houndci-bot Aug 16, 2018

at least two spaces before inline comment

@GClunies

This comment has been minimized.

GClunies commented Aug 20, 2018

@jcconnell, just as a heads up... py_noaa has been updated to version 0.3.0 to incorporate your PR and a few others.

@jcconnell

This comment has been minimized.

Contributor

jcconnell commented Aug 21, 2018

Thanks @GClunies, just incremented the version!

@@ -739,6 +739,9 @@ pyTibber==0.4.1
# homeassistant.components.switch.dlink
pyW215==0.6.0
# homeassistant.components.sensor.noaa_tides
# py_noaa==0.2.2

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 22, 2018

Member

Run gen_requirements.py again to sync the requirements.

@MartinHjelmare

@fabaff are you happy with the time formatting and time zones?

@fabaff

This comment has been minimized.

Member

fabaff commented Aug 22, 2018

The high_tide_time and low_tide_time will most likely be used in templates and providing an ISO 8601 formatted timestamp will make it simpler to work with them.

According to NOAA website are the times in LST/LDT. I have no idea on how to make them aware of the timezone because Home Assistant could be located in a different zone which will lead to deviations. Let's solve this when people start complaining. It looks like that this is addressed by using timezone:.

@fabaff

fabaff approved these changes Aug 22, 2018

@fabaff fabaff merged commit 4155e8a into home-assistant:dev Aug 22, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 93.778%
Details

@wafflebot wafflebot bot removed the in progress label Aug 22, 2018

@jcconnell

This comment has been minimized.

Contributor

jcconnell commented Aug 22, 2018

Thank you all for the help and patience! I hope other users find this helpful. I got tired of receiving API credit exhausted errors with World Tides.

@balloob balloob referenced this pull request Aug 29, 2018

Merged

0.77 #16256

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Add support for NOAA tide information (new PR) (home-assistant#15947)
* Adding noaa-tides changes to new branch.

* Fix typo in .coverageac

* Incorporate @MartinHjelmare and @fabaff changes.

* Disable pylint error and add error message for unavailable station.

* Two spaces before inline comments

* Increment py_noaa version to 0.3.0

* Updated requirements.py

* Minor changes

@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018

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