Add new nextbus sensor

[ViViDboarder]

Description:

New platform sensor to display upcoming transit arrivals using NextBus.

This is my first contribution to hass, so I'm submitting for some early feedback while there is still a little work to be done. I need to write tests, documentation, (no longer waiting for patch to be merged into py_nextbus to fix an issue with the client, instead we're working around it.)

The other thing I'm considering is allowing to add the platform once, but then provide a list of routes, stops, and names so that the platform can make a single request to update all sensors at once. I'm not sure what is the typical convention here though and chose to implement the simplest thing first. Looking for feedback on design.

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

Example entry for configuration.yaml (if applicable):

sensors:
  - platform: nextbus
    agency: sf-muni  # agency tag from NextBus
    route: F         # route tag from NextBus
    stop: 35184      # stop tag from NextBus
    name: 'My stop'  # optional name to use for the sensor

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.

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

• Documentation added/updated in home-assistant.io

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.

If the code does not interact with devices:

• Tests have been added to verify that the new code works.

[ViViDboarder]

Just removed dependency on upstream patch and added documentation.

[ViViDboarder]

So the Travis build is confusing me. It seems that pylint is complaining about:

# Validate agencies
if not validate_value(
    'agency',
    agency,
    client.get_agency_list()['agency'],
):

However this appears to conform to pep8 just fine. It also seems that flake8 agrees that this is ok.

It is asking for the internal values to be double indented as so:

# Validate agencies
if not validate_value(
        'agency',
        agency,
        client.get_agency_list()['agency'],
):

It allows a 4 space hanging indent in other places, but it seems to be asking for 8 spaces when there is a function continuation within an if statement. This ends up passing pylint and flake8, so I can make that change, but I've never seen this convention and wanted to check.

It looks like it may just be a bug in pylint.

[ViViDboarder]

Just added tests. Haven't figured out how to successfully mock instance methods on an object within a dependency module... So that test is skipped.

Also, because I couldn't figure this out, I had to move the imports into some module level functions so that they could be mocked out entirely without having to mock the client itself.

I am not a fan of the way I've got it working right now. I'd prefer if I didn't have to update the actual module to support tests and wish I could keep it looking more like it did in this diff

[ViViDboarder]

Removed module from the omit section of .coveragerc since it has 80%+ coverage.

Also, it doesn't look like I can use pytest parameterized tests with unittest, so the tests look a little verbose. I'm using unittest because it appears to be the standard practice here, but I'm happy to refactor into something purely dependent on pytest.

[ViViDboarder]

Odd that tests ran fine on my branch. I just ran tests after merging dev into this and it seems to pass.

Anything more that I need to do here? I see a few labels that are out of date now.

[MartinHjelmare]

Please rebase on latest dev branch, to solve the merge conflict.

[ViViDboarder]

Wow. Looks like things have changed quite a bit since I last submitted a patch.

I'll try to update using the new conventions and add a manifest file.

[ViViDboarder]

Ok. Think I finally got through all the checks. For some reason some tox envs failed to run locally preventing me from catching some checks until CircleCI ran. Eg:

lint run-test: commands[0] | python script/gen_requirements_all.py validate
Traceback (most recent call last):
  File "script/gen_requirements_all.py", line 11, in <module>
    from script.hassfest.model import Integration
ModuleNotFoundError: No module named 'script'

Which prevented me from finding the requirements ordering issue.

I believe that's because tox.ini is executing python script/gen_requirements_all.py validate rather than python -m script.gen_requirements_all validate, like it does in the automated tests.

[ViViDboarder]

Woo! Finally all green! :)

Let me know if there's anything you'd like me to update.

[MartinHjelmare]

Please don't squash commits after review has started. Sqaushing commits makes it harder for readers to track changes.

[ViViDboarder]

Sure. Good feedback. I originally did it because I had to rebase ~25 commits onto dev and haven't mastered git rerere. I'll be more careful next time for sure.

[MartinHjelmare]

It would have been ok to squash everything until before my last review, but keep the last changes as separate commits. Then I could have just looked at those. Now I have to look over everything again.

[MartinHjelmare]

Good! Can be merged when last comment is addressed and build passes.

[ViViDboarder]

Thanks!

Just when I thought I was done, found another edge case with this API. Really feeling the burn of an untyped and not well documented API.

Just found out that like the message field that can be a dict or a list of dicts, so can direction and it's child, prediction. So now I have to handle them all as either dicts or list of dicts.

Adding more tests to cover this permutation.

[ViViDboarder]

Ok. Made my changes and working on an update. I'm struggling a bit with the actual interface now. It doesn't look very friendly at all as it just renders the ISO timestamp rather than the actual wait times.

I'm not sure what's missing as I followed the example you linked to. How can I make it show the relative time until the bus/train leaves? Setting device_class doesn't seem to have had an effect.

[MartinHjelmare]

Use lovelace. Eg with the system monitor sensor and yaml lovelace config:

type: entities
entities:
  - entity: sensor.last_boot
    format: relative
show_header_toggle: false

https://www.home-assistant.io/lovelace/entities/#format

It will default to relative so you don't need to go into yaml mode if relative is fine.

[ViViDboarder]

It doesn't seem to have defaulted to relative for me on a brand new install. It's showing a long ISO string that overflows so all I can see is the year.

Is there documentation on how to use timestamp sensors for users? When I look at the the docs for the last_boot sensor, it doesn't link to or describe how to make it show a relative time. Is there a way for me to set the default for the user? This change to the default rendering is not very intuitive. Would you be open to a new PR changing the default formatting for timestamps?

[MartinHjelmare]

We should keep the timestamp device class and state formatting. Currently you have to be in managed lovelace mode to be able to add the correct card type. This could possible be enhanced in the frontend.

[MartinHjelmare]

Can be merged when build passes.