Skip to content

Add Norwegian Public Transportation sensor (Ruter).#18237

Merged
balloob merged 6 commits intohome-assistant:devfrom
ludeeus:ruter
Nov 6, 2018
Merged

Add Norwegian Public Transportation sensor (Ruter).#18237
balloob merged 6 commits intohome-assistant:devfrom
ludeeus:ruter

Conversation

@ludeeus
Copy link
Copy Markdown
Member

@ludeeus 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.

Copy link
Copy Markdown
Contributor

@amelchio amelchio left a comment

Choose a reason for hiding this comment

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

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

@ludeeus
Copy link
Copy Markdown
Member Author

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
@ghost ghost removed the in progress label Nov 6, 2018
@balloob
Copy link
Copy Markdown
Member

balloob commented Nov 6, 2018

@amelchio you can merge if you approve 👍

@ludeeus ludeeus deleted the ruter branch November 6, 2018 18:51
name = config[CONF_NAME]
offset = config[CONF_OFFSET]

ruter = Departures(hass.loop, stop_id, destination)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah no, we wouldn't do that.

@1v4r
Copy link
Copy Markdown

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
Copy link
Copy Markdown
Member

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants