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 london_underground #8272

Merged
merged 15 commits into from
Jul 2, 2017
Merged

Add london_underground #8272

merged 15 commits into from
Jul 2, 2017

Conversation

robmarkcole
Copy link
Contributor

@robmarkcole robmarkcole commented Jul 1, 2017

Description:

This sensor displays the current status of London Underground tube lines. If there is a disruption, a description of the disruption is viewable as an attribute.
This pull request replaces #8235

Example entry for configuration.yaml:

sensor:
  - platform: london_underground
    line:
      - Bakerloo
      - Central
      - Circle
      - District
      - DLR
      - Hammersmith & City
      - Jubilee
      - London Overground
      - Metropolitan
      - Northern
      - Piccadilly
      - TfL Rail
      - Victoria
      - Waterloo & City

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

Checklist:

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Add tube_state sensor
'Waterloo & City']
URL = 'https://api.tfl.gov.uk/line/mode/tube,overground,dlr,tflrail/status'

CONFIG_SCHEMA = vol.Schema({
Copy link
Member

Choose a reason for hiding this comment

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

You are a platform so please define a platform schema:

PLATFORM_SCHEMA = vol.Schema({
    vol.Required(CONF_LINE): vol.In(TUBE_LINES),
})


ATTRIBUTION = "Powered by TfL Open Data"
CONF_LINE = 'line'
DOMAIN = 'tube_state'
Copy link
Member

Choose a reason for hiding this comment

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

platforms don't have a domain. Please remove this line.

sensors.append(LondonTubeSensor(line, data))

add_devices(sensors, True)
_LOGGER.info("The tube_state component is ready!")
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these two log lines.

def update(self):
"""Get the latest data from TFL."""
response = requests.get(URL)
_LOGGER.info("TFL Request made")
Copy link
Member

Choose a reason for hiding this comment

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

Please don't log this as info. If it needs to be logged at all, make it debug

"""Stop everything that was started."""
self.hass.stop()

def test_setup_with_config(self):
Copy link
Member

Choose a reason for hiding this comment

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

Please merge this test with the one below. So instead of tube_state.setup_platform(…) just call setup_component(…).

Then you can read the state from the state machine.

state = self.hass.states.get('sensor.london_overground')
assert state.state == 'bla'
assert state.attributes.get('Description') == 'Something'

@balloob balloob added this to the 0.48 milestone Jul 1, 2017
Correct PLATFORM_SCHEMA
self.hass.stop()


@requests_mock.Mocker()

Choose a reason for hiding this comment

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

too many blank lines (2)

@balloob
Copy link
Member

balloob commented Jul 1, 2017

🎉 🎉 🐬

@lwis
Copy link
Member

lwis commented Jul 1, 2017

@robmarkcole self.hass.states.get('sensor.london_overground') is returning None.

@robmarkcole
Copy link
Contributor Author

robmarkcole commented Jul 1, 2017

I don't understand why self.hass.states.get('sensor.london_overground') is returning None..
Test does pass with:

sensor = self.entities[0]
self.assertEqual(sensor.name, 'London Overground')
self.assertEqual(sensor.state, 'Minor Delays')
self.assertEqual(sensor.device_state_attributes['Description'], 'something')

Preferred method below returns None

state = self.hass.states.get('sensor.london_overground')
sensor = self.entities[0]
self.assertEqual(sensor.name, 'London Overground')
self.assertEqual(sensor.state, 'Minor Delays')
self.assertEqual(sensor.device_state_attributes['Description'], 'something')

Choose a reason for hiding this comment

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

line too long (84 > 79 characters)

setup_component(self.hass, 'sensor', {'tube_state': self.config}))
#state = self.hass.states.get('sensor.london_overground')
#assert state.state == 'Minor Delays'
#assert state.attributes.get('Description') == 'something'

Choose a reason for hiding this comment

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

block comment should start with '# '

self.assertTrue(
setup_component(self.hass, 'sensor', {'tube_state': self.config}))
#state = self.hass.states.get('sensor.london_overground')
#assert state.state == 'Minor Delays'

Choose a reason for hiding this comment

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

block comment should start with '# '

self.assertEqual(len(self.entities), 1)
self.assertTrue(
setup_component(self.hass, 'sensor', {'tube_state': self.config}))
#state = self.hass.states.get('sensor.london_overground')

Choose a reason for hiding this comment

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

block comment should start with '# '

self.assertEqual(sensor.name, 'London Overground')
self.assertEqual(sensor.state, 'Minor Delays')
self.assertEqual(sensor.device_state_attributes['Description'],
'something')

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

# assert state.state == 'Minor Delays'
# assert state.attributes.get('Description') == 'something'

sensor = self.entities[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This passes. Copied technique from test_wsdot.py

Copy link
Member

Choose a reason for hiding this comment

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

This technique should not be used. please remove.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

The test needs fixing.

class TestLondonTubeSensor(unittest.TestCase):
"""Test the tube_state platform."""

def add_entities(self, new_entities, update_before_add=False):
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

"""Initialize values for this testcase class."""
self.hass = get_test_home_assistant()
self.config = {CONF_LINE: ['London Overground']}
self.entities = []
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

def test_setup(self, mock_req):
"""Test for operational tube_state sensor with proper attributes."""
mock_req.get(URL, text=load_fixture('tube_state.json'))
tube_state.setup_platform(self.hass, self.config, self.add_entities)
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

"""Test for operational tube_state sensor with proper attributes."""
mock_req.get(URL, text=load_fixture('tube_state.json'))
tube_state.setup_platform(self.hass, self.config, self.add_entities)
self.assertEqual(len(self.entities), 1)
Copy link
Member

Choose a reason for hiding this comment

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

And this.

self.assertEqual(len(self.entities), 1)
self.assertTrue(
setup_component(self.hass, 'sensor', {'tube_state': self.config}))
# state = self.hass.states.get('sensor.london_overground')
Copy link
Member

Choose a reason for hiding this comment

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

Instead of commenting it out, check what the entity name is. It's right there in the test logs.

# assert state.state == 'Minor Delays'
# assert state.attributes.get('Description') == 'something'

sensor = self.entities[0]
Copy link
Member

Choose a reason for hiding this comment

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

This technique should not be used. please remove.

@balloob balloob removed this from the 0.48 milestone Jul 1, 2017
Test fails with:

AssertionError: assert 0 > 0
where 0 = len([])

Surely I need tube_state.setup_platform ?
import unittest
import requests_mock

from homeassistant.components.sensor import tube_state

Choose a reason for hiding this comment

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

'homeassistant.components.sensor.tube_state' imported but unused

setup_component(self.hass, 'sensor', {'tube_state': self.config}))

ids = self.hass.states.entity_ids()
assert len(ids) > 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have zero entities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import unittest
import requests_mock

from homeassistant.components.sensor import london_tube

Choose a reason for hiding this comment

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

'homeassistant.components.sensor.london_tube' imported but unused

import unittest
import requests_mock

from homeassistant.components.sensor import london_tube

Choose a reason for hiding this comment

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

'homeassistant.components.sensor.london_tube' imported but unused

@robmarkcole robmarkcole changed the title Add tube_state Add london_tube Jul 2, 2017
import unittest
import requests_mock

from homeassistant.components.sensor import london_underground

Choose a reason for hiding this comment

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

'homeassistant.components.sensor.london_underground' imported but unused

@robmarkcole robmarkcole changed the title Add london_tube Add london_underground Jul 2, 2017
@robmarkcole
Copy link
Contributor Author

I'm not sure what needs to be done about the failing Travis @ https://travis-ci.org/home-assistant/home-assistant/jobs/249272537

@balloob
Copy link
Member

balloob commented Jul 2, 2017

Woohooo 🎉

@balloob balloob merged commit 865865c into home-assistant:dev Jul 2, 2017
@robmarkcole
Copy link
Contributor Author

Fantastic 🤗

@robmarkcole robmarkcole deleted the london_tube_state branch July 2, 2017 18:37
@balloob balloob mentioned this pull request Jul 13, 2017
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
* Add tube_state

Add tube_state sensor

* Final cleanup

* Make corrections

Correct PLATFORM_SCHEMA

* Fix space

* Make test pass

* Correct format of test

Test still failing, don’t understand why

* correct description

* Make test pass

Preferred method below returns None

state = self.hass.states.get('sensor.london_overground')

* Format for hound

* indent

* Make requested changes to test, not working

Test fails with:

AssertionError: assert 0 > 0
where 0 = len([])

Surely I need tube_state.setup_platform ?

* Fixed test

Config was wrong

* Change component name to london_tube

* Update name to london_underground

Make consistent

* cleanup
@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants