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 NMBS (Belgian railway) sensor platform #18610

Merged
merged 23 commits into from Dec 14, 2018

Conversation

Projects
None yet
8 participants
@thibmaek
Copy link
Contributor

thibmaek commented Nov 21, 2018

Description:

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: nmbs
    station_from: "Brugge"
    station_to: "Gent Dampoort"
    station_live: "Brugge"

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.

thibmaek added some commits Nov 20, 2018

@thibmaek

This comment has been minimized.

Copy link
Contributor

thibmaek commented Nov 21, 2018

Will add docs once I know everything in this PR is ok.

@frenck frenck added the docs-missing label Nov 21, 2018

@thibmaek thibmaek referenced this pull request Nov 24, 2018

Merged

Add docs for NMBS sensor #7622

2 of 2 tasks complete
@thibmaek

This comment has been minimized.

Copy link
Contributor

thibmaek commented Nov 24, 2018

Documentation PR is open for this :)

@ludeeus ludeeus removed the docs-missing label Nov 24, 2018

@fabaff
Copy link
Member

fabaff left a comment

Just some quick comments to get the review started.

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

thibmaek added some commits Nov 25, 2018

@ludeeus

This comment has been minimized.

Copy link
Member

ludeeus commented Nov 27, 2018

You are mixing your quotes throughout the file, use singe ' for single word, and double " for multiple words or sentences.
https://developers.home-assistant.io/docs/en/development_guidelines.html#quotes

@thibmaek

This comment has been minimized.

Copy link
Contributor

thibmaek commented Nov 28, 2018

You are mixing your quotes throughout the file, use singe ' for single word, and double " for multiple words or sentences.
developers.home-assistant.io/docs/en/development_guidelines.html#quotes

Just corrected that too :)

Show resolved Hide resolved .coveragerc Outdated
Show resolved Hide resolved .coveragerc
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py
Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
@thibmaek

This comment has been minimized.

Copy link
Contributor

thibmaek commented Dec 9, 2018

I'm having mixed feelings about this PR. Seems like nothing I do is good enough and remarks keep on coming (even being contradictory). I'll consider what I will do with this PR, not sure if I still want to add this to HA core.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 9, 2018

I encourage you to keep working on the PR. The comments and discussion in the PR review are normal and part of the work we do as contributors. I think you're a doing a great job. 👍

thibmaek added some commits Dec 10, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into feature/nmbs-sensor

* 'dev' of https://github.com/home-assistant/home-assistant: (325 commits)
  Update translations
  Updated frontend to 20181210.0
  Lovelace using storage (#19101)
  Update pygtfs to upstream's 0.1.5 (#19151)
  Update radiotherm to 2.0.0 and handle change in tstat error detection (#19107)
  Upgrade sphinx-autodoc-typehints to 1.5.2 (#19140)
  Update geizhals dependency (#19152)
  Upgrade mypy to 0.650 (#19150)
  Upgrade upcloud-api to 0.4.3
  Upgrade youtube_dl to 2018.12.03 (#19139)
  Upgrade slacker to 0.12.0
  Add code support for iAlarm (#19124)
  Fixed doorbird config without events (empty list) (#19121)
  Remove marking device tracker stale if state is stale (#19133)
  Update Google Assistant services description and request sync timeout (#19113)
  update edp_redy version (#19078)
  Add Philips Moonlight Bedside Lamp support (#18496)
  Upgrade Mill library (#19117)
  Don't avoid async_schedule_update_ha_state by returning false (#19102)
  Fix the Xiaomi Aqara Cube rotate event of the LAN protocol 2.0 (Closes: #18199) (#19104)
  ...
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Looks good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 10, 2018

Can be merged when build passes.

thibmaek added some commits Dec 10, 2018

Merge branch 'dev' of github.com:thibmaek/home-assistant into feature…
…/nmbs-sensor

* 'dev' of github.com:thibmaek/home-assistant:
  Add raw service data to event (#19163)
  Updated frontend to 20181210.1
  Google assistant fix target temp for *F values. (#19083)
  Fix lovelace save (#19162)
  Drop OwnTracks bad packets (#19161)
  #18645: revert heat-cool -> auto change
  #18645: Remove un-used constants.
  #18645: Fix climate mode mapping.
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Dec 10, 2018

Did you run the gen_requirements.py script and commit the changes? The requirements_all.txt file should after that have an entry with the new library.

@tjorim
Copy link
Contributor

tjorim left a comment

I might be wrong but just wanted to double-check it.

Show resolved Hide resolved homeassistant/components/sensor/nmbs.py Outdated
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Those are the fixes highlighted by Travis.

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

tjorim and others added some commits Dec 11, 2018

Update homeassistant/components/sensor/nmbs.py
Co-Authored-By: thibmaek <thibault.maekelbergh@iCloud.com>
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Nice! Can be merged when build passes.

@thibmaek

This comment has been minimized.

Copy link
Contributor

thibmaek commented Dec 14, 2018

PS: I've updated the documentation PR to set the released version to 0.85

@MartinHjelmare MartinHjelmare merged commit 4a23d4c into home-assistant:dev Dec 14, 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 Coverage decreased (-0.01%) to 92.965%
Details

@wafflebot wafflebot bot removed the in progress label Dec 14, 2018

dshokouhi added a commit to dshokouhi/home-assistant that referenced this pull request Dec 25, 2018

Add NMBS (Belgian railway) sensor platform (home-assistant#18610)
* Add custom component to core

* Add pyrail to reqs

* Format & lint

* Sort nmbs.py into place on coveragerc

* Only set up station live if provided

* Only set up sensor if config is provided

* Fix line too long linting error

* PR Remarks

* Add docstrings

* Fix hound line to long error

* Fix quotes

* Rebase coveragerc

* PR Review

* Init empty attrs

* Dont include delay if there is none

* PR review

* Safer check

* Rebase reqs

* Generate req

* Update homeassistant/components/sensor/nmbs.py

Co-Authored-By: thibmaek <thibault.maekelbergh@iCloud.com>

* PR remarks

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

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