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 Norwegian Public Transportation sensor (Ruter). #18237

Merged
merged 6 commits into from Nov 6, 2018

Conversation

@ludeeus
Member

ludeeus commented Nov 5, 2018

Description:

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

Example entry for configuration.yaml (if applicable):

sensor:
  platform: ruter
  stopid: 2190400

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.

ludeeus added some commits Nov 5, 2018

@amelchio

Looks good, I just have a few proposals for slight cleanups.

Show resolved Hide resolved homeassistant/components/sensor/ruter.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/ruter.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/ruter.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/ruter.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/ruter.py
change stopid to stop_id, actually use attributes, changed logging, c…
…orrected link, removed unused variable.
@ludeeus

This comment has been minimized.

Member

ludeeus commented Nov 5, 2018

Thanks @amelchio :)
I think I caught all requests :) have also updated the docs to match the change from stopid to stop_id

@balloob balloob merged commit c41ca37 into home-assistant:dev Nov 6, 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 First build on ruter at 93.062%
Details

@wafflebot wafflebot bot removed the in progress label Nov 6, 2018

@balloob

This comment has been minimized.

Member

balloob commented Nov 6, 2018

@amelchio you can merge if you approve 👍

@ludeeus ludeeus deleted the ludeeus:ruter branch Nov 6, 2018

name = config[CONF_NAME]
offset = config[CONF_OFFSET]
ruter = Departures(hass.loop, stop_id, destination)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 7, 2018

Member

We want all libraries that use aiohttp to take a session argument so that we can reuse the home assistant session throughout the app.

This comment has been minimized.

@ludeeus

ludeeus Nov 7, 2018

Member

I was not aware of that, will change it that and create a new PR later today :)

This comment has been minimized.

@amelchio

amelchio Nov 7, 2018

Member

Isn't it more like "we would like"? I believe that part of the deal with using third party libraries is that we don't get to dictate their interface.

Still, @ludeeus, please do continue with implementing that :)

This comment has been minimized.

@balloob

balloob Nov 7, 2018

Member

@amelchio we don't have to dictate their interface, but very much like if they do take in an optional aiohttp session instead of creating their own if they use aiohttp.

This comment has been minimized.

@amelchio

amelchio Nov 7, 2018

Member

Yes, and for a library created for Home Assistant that is no biggie. I was more concerned about rejecting a new platform because an existing library does not support passing in the session.

This comment has been minimized.

@balloob

balloob Nov 7, 2018

Member

Ah no, we wouldn't do that.

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Add Norwegian Public Transportation sensor (Ruter). (home-assistant#…
…18237)

* Add Norwegian Public Transportation sensor (Ruter).

* Corrected typo.

* change stopid to stop_id, actually use attributes, changed logging, corrected link, removed unused variable.

* Change to RuterSensor for the class, and move logic to me more readable.

* Use correct sensor class.

* Add return if blank list, remove else

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

@1v4r

This comment has been minimized.

1v4r commented Nov 30, 2018

Very cool to see this implemented in Home Assistant.
Will this be updated to use the Entur-API once the Ruter API is terminated? That would be cool as Entur includes the full national public transport service. Not just Oslo and Akershus.
https://ruter.no/labs/

@Danielhiversen

This comment has been minimized.

Member

Danielhiversen commented Nov 30, 2018

Entur is integrated:
#17286

Please, do not used closed pr for discussion.

@home-assistant home-assistant locked and limited conversation to collaborators Nov 30, 2018

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