Skip to content

Platform for Munich public transport departure times#6704

Merged
fabaff merged 6 commits into
home-assistant:devfrom
DavidMStraub:mvglive
Mar 26, 2017
Merged

Platform for Munich public transport departure times#6704
fabaff merged 6 commits into
home-assistant:devfrom
DavidMStraub:mvglive

Conversation

@DavidMStraub
Copy link
Copy Markdown
Contributor

@DavidMStraub DavidMStraub commented Mar 19, 2017

I'm not sure if this is too local to become an official component but for me it's very useful so I thought I'd share it anyway.

Description:

The MVG live service (http://www.mvg-live.de) provides real-time departure information for the Munich public transort system (bus, tram, subway, suburban rail). This platform provides a sensor for the time to the next departure (optionally the next departure after an offset, e.g. the time it takes you to walk to the bus stop) at a stop or station. The destination and line number are given in the attributes. Optionally, specific lines or destinations can be selected.

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

Example entry for configuration.yaml (if applicable):

sensor:
  platform: mvglive
  station: Marienplatz

Checklist:

  • Documentation added/updated in home-assistant.github.io
  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@mention-bot
Copy link
Copy Markdown

@DavidMStraub, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @rmkraus to be potential reviewers.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 19, 2017

Nothing is too local 👍

Copy link
Copy Markdown
Member

@balloob balloob Mar 19, 2017

Choose a reason for hiding this comment

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

Make sure to open an issue for this on their repo so we can migrate to use the PyPi version in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, just replaced it with the PyPI version

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Few changes, code looks good 👍

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.

Please don't call update here but instead pass True as a second parameter to add_devices

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.

It's usually not a good idea to mutate data that you fetch from a library. They might store a reference in a cache that is now no longer what they expect it to be. Create new dictionaries instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hints! I think I fixed everything.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

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.

This code is unmaintainable. Please make it a for loop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. Refactored and simplified.

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.

This case can never happen because you always have an empty dictionary in your list, meaning it will always test truthy. (only an empty list is False)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right. Refactored the code now - in fact, no need for the list as we're only interested in the 1st item.

continue
# now select the relevant data
_nextdep = {}
for k in ['destination', 'linename', 'time', 'direction', 'product']:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

@fabaff fabaff dismissed balloob’s stale review March 26, 2017 17:06

All comments are addressed.

Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

🐦

@fabaff fabaff merged commit 78b5eb7 into home-assistant:dev Mar 26, 2017
@fabaff fabaff mentioned this pull request Apr 6, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Jul 17, 2017
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.

7 participants