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 Rejseplanen danish public transport sensor component #19885

Merged
merged 7 commits into from Feb 12, 2019

Conversation

Projects
None yet
6 participants
@tomatpasser
Copy link
Contributor

tomatpasser commented Jan 9, 2019

Description:

This pull request add support for a sensor for the danish Rejseplanen public transport service.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: rejseplanen
    stop_id: '000045740'
    route: 'Bus 350S'
    direction:
      - 'Herlev St.'
      - 'Ballerup St.'

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 has been minimized.

Copy link

homeassistant commented Jan 9, 2019

Hi @tomatpasser,

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!

Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated

@homeassistant homeassistant added cla-signed and removed cla-needed labels Jan 9, 2019

@tomatpasser

This comment has been minimized.

Copy link
Contributor Author

tomatpasser commented Jan 9, 2019

I apologize for the commented out code and PEP style violations, fixing that now. Do I need to make a new PR or can i amend it?

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Jan 9, 2019

Just push additional commits to this PR's branch

@tomatpasser

This comment has been minimized.

Copy link
Contributor Author

tomatpasser commented Jan 9, 2019

Alright fixed that. Do I need to write a test for this to be merged?

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Jan 10, 2019

Yeah, I think so.
You can find inspiration for other transport sensors here: https://github.com/home-assistant/home-assistant/tree/dev/tests/components/sensor

@amelchio
Copy link
Member

amelchio left a comment

Tests are not required for a new sensor platform.

However, all API code (the HTTP call and extraction of returned data) must be moved to a separate PyPI package that is added as a REQUIREMENT, see the checklist: https://developers.home-assistant.io/docs/en/creating_platform_code_review.html

@tomatpasser

This comment has been minimized.

Copy link
Contributor Author

tomatpasser commented Jan 10, 2019

OK. Did not notice that, as other transport sensors have API code in them.
So how much code is allowed here? If the PyPI package just returns everything, can it be filtered in the component?

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Jan 11, 2019

Ask not how much code is allowed – ask how much can be removed ;-)

It is preferable to make a quality PyPI package that can be used for other projects as well.

tomatpasser added some commits Feb 4, 2019

@tomatpasser

This comment has been minimized.

Copy link
Contributor Author

tomatpasser commented Feb 4, 2019

I wrote a PyPI package which implements the full specification I have of the API. How does this look?

@amelchio
Copy link
Member

amelchio left a comment

This looks a lot better.

I would have liked the library to format the information as Python objects so one could write more readable and less error-prone things like self._times[0][ATTR_DUE_IN]self._times.next.due_in. Also, the result filtering and due_at calculation could easily go into the library.

That said, the official policy is that we do not dictate the design of third party libraries and I think the current implementation is acceptable.

Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
@tomatpasser

This comment has been minimized.

Copy link
Contributor Author

tomatpasser commented Feb 7, 2019

Thanks for the feedback, also on the library. I have committed the changes.

Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated

try:
results = rjpl.departureBoard(int(self.stop_id), timeout=5)
except RuntimeError:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 7, 2019

Member

What error are we expecting here? The library should preferably raise its own exceptions, that we can import and catch here.

This comment has been minimized.

@tomatpasser

tomatpasser Feb 7, 2019

Author Contributor

Possible errors are connection errors or any error returned by the API.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 7, 2019

Member

We only need to catch expected errors. I think RuntimeError is too broad. Try to limit it to specific error types. If there are api specific errors, the library should catch those and raise its own exceptions.

This comment has been minimized.

@tomatpasser

tomatpasser Feb 7, 2019

Author Contributor

I have created 3 errors now:

  • APIError (any error from the API such as no routes found etc.)
  • Connection error
  • HTTP error

How does it look?

This comment has been minimized.

@tomatpasser

tomatpasser Feb 7, 2019

Author Contributor

I will finish this up with the other improvements then.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 7, 2019

Member

Good! I think there is risk for name clashes though with some existing errors. Eg ConnectionError is already in standard library. I would name the errors something with rjpl in the name. Here's some more good reading I would recommend:
https://julien.danjou.info/python-exceptions-guide/

This comment has been minimized.

@tomatpasser

tomatpasser Feb 7, 2019

Author Contributor

Thanks for the resource! I renamed the exceptions to have rjpl in the name.

Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/rejseplanen.py Outdated
@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Feb 7, 2019

My comments have been addressed, thanks.

Obviously we should consider Martin's comments as well before merging.

@tomatpasser

This comment has been minimized.

Copy link
Contributor Author

tomatpasser commented Feb 7, 2019

@MartinHjelmare I hope that I addressed your comments appropriately in the recent commits.

@amelchio

This comment has been minimized.

Copy link
Member

amelchio commented Feb 11, 2019

@MartinHjelmare This looks good to me now, do you agree?

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Yes!

@MartinHjelmare MartinHjelmare merged commit 69df620 into home-assistant:dev Feb 12, 2019

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 Coverage decreased (-0.01%) to 93.353%
Details

@wafflebot wafflebot bot removed the in progress label Feb 12, 2019

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