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 new nextbus sensor #20197

Merged
merged 4 commits into from Apr 27, 2019

Conversation

@ViViDboarder
Copy link
Contributor

commented Jan 17, 2019

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:

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

This comment was marked as resolved.

Copy link

commented Jan 17, 2019

Hi @ViViDboarder,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
@homeassistant

This comment was marked as resolved.

Copy link

commented Jan 17, 2019

Hi @ViViDboarder,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Just removed dependency on upstream patch and added documentation.

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

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.

tests/components/sensor/test_nextbus.py Outdated Show resolved Hide resolved
tests/components/sensor/test_nextbus.py Outdated Show resolved Hide resolved
tests/components/sensor/test_nextbus.py Outdated Show resolved Hide resolved

@ViViDboarder ViViDboarder force-pushed the ViViDboarder:nextbus branch from 6c9971a to 7485eef Feb 22, 2019

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

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.

homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
tests/components/sensor/test_nextbus.py Outdated Show resolved Hide resolved
homeassistant/components/sensor/nextbus.py Outdated Show resolved Hide resolved
requirements_all.txt Outdated Show resolved Hide resolved
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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

@MartinHjelmare MartinHjelmare changed the title Added new nextbus sensor Add new nextbus sensor Mar 28, 2019

@ViViDboarder ViViDboarder force-pushed the ViViDboarder:nextbus branch from 65176a8 to c3c0ab7 Mar 28, 2019

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

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.

@home-assistant home-assistant deleted a comment from houndci-bot Apr 23, 2019

@home-assistant home-assistant deleted a comment from houndci-bot Apr 23, 2019

tests/components/nextbus/test_sensor.py Outdated Show resolved Hide resolved
tests/components/nextbus/test_sensor.py Outdated Show resolved Hide resolved
tests/components/nextbus/test_sensor.py Outdated Show resolved Hide resolved
tests/components/nextbus/test_sensor.py Outdated Show resolved Hide resolved
tests/components/nextbus/test_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/nextbus/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/nextbus/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/nextbus/sensor.py Outdated Show resolved Hide resolved

@ViViDboarder ViViDboarder force-pushed the ViViDboarder:nextbus branch 2 times, most recently from 69a4710 to 33836b9 Apr 23, 2019

@ViViDboarder ViViDboarder force-pushed the ViViDboarder:nextbus branch from 33836b9 to df93bec Apr 23, 2019

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Woo! Finally all green! :)

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

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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

left a comment

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

homeassistant/components/nextbus/sensor.py Outdated Show resolved Hide resolved
@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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.

tox.ini Outdated Show resolved Hide resolved
@ViViDboarder

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

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

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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.

@ViViDboarder ViViDboarder referenced this pull request Apr 24, 2019
3 of 3 tasks complete
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Can be merged when build passes.

@andrewsayre andrewsayre merged commit c2e7445 into home-assistant:dev Apr 27, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 95.09% of diff hit (target 94.31%)
Details
codecov/project 94.31% (target 90%)
Details
@balloob balloob referenced this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.