8000 mvglive bug fixes and improvements by mountainsandcode · Pull Request #6953 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

mvglive bug fixes and improvements #6953

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

Merged
merged 4 commits into from
Apr 20, 2017
Merged

Conversation

mountainsandcode
Copy link
Contributor
@mountainsandcode mountainsandcode commented Apr 5, 2017

Description:

@DavidMStraub provided a first implementation of public transport departures in Munich through MVG-live.de. This pull requests carries forward this work by:

  • Refactoring the code so that multiple sensors for departures can be added
  • Rewrites the transport mode restrictions ("products") to be more modular and allows to filter U-Bahn departures by direction
  • Fixes bugs: Configuration of line restrictions has no effect on sensor due to missing implementation
  • Other improvements, such as including data attribution and flexible icons

The change breaks the configuration, however, since it is currently still in dev, this should be acceptable.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: mvglive
    nextdeparture:
     - station: Hauptbahnhof
       name: Hbf
       destinations: ['München Flughafen Terminal','Markt Schwaben']
       products: 'S-Bahn'
       timeoffset: 2
     - station: Sendlinger Tor
       lines: ['U2','U8']
     - station: Scheidplatz
       products: ['U-Bahn']
       directions: '1'

Checklist:

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

If the code does not interact with devices:

  • [ Local tests with tox run successfully. Your PR cannot be merged unless tests pass
    (Have had some trouble getting tox to run locally - Travis tests appear to pass)

This pull requests builds on the first work with the mvglive 
8000
sensor:
- Refactoring the code so that multiple sensors for departures can be added
- Rewrites the transport mode restrictions ("products") to be more modular
- Fixes bugs, such as missing implementation of line restriction
- Other improvements, such as including data attribution
@mention-bot
Copy link

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

@homeassistant
Copy link
Contributor

Hi @mountainsandcode,

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!

- The API returns the property 'direction', which can be used to filter U-Bahn trains by direction without having to enter all final destinations
- The sensor icon now corresponds to the mode of transport of the next departure
else:
self._state = self.data.nextdeparture.get('time', '-')
self._state = self.data.departures.get('time', '-')
self._icon = ICONS_PRODUCTS[self.data.departures.get('product', '-')]

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)

@mountainsandcode mountainsandcode changed the title mvglive improvements mvglive bug fixes and improvements Apr 6, 2017
U-Bahn SEV (bus replacement services) have unexpected direction values, fixed resulting bug and hound issues
@DavidMStraub
Copy link
Contributor

Hi, thanks for the fixes and improvements. What is the rationale for the nextdeparture argument?

@DavidMStraub
Copy link
Contributor

By the way, the previous version is part of the PR #6956 for 0.42 which I guess will be merged on the weekend, so unless this PR can be merged in time probably the whole platform should be postponed to 0.43...

@mountainsandcode
Copy link
Contributor Author
mountainsandcode commented Apr 6, 2017

@DavidMStraub - Agree on your reasoning regarding release since there doesn't seem much of a point releasing something if configuration will be broken in the next release. From my side I think this should be good to go already for 0.42 (have been testing the different combination of options during the afternoon), so if you don't see any problems with the changes and the maintainers are happy with code quality, maybe this is good to go already for this release.

With regards to the nextdeparture argument, I am hoping that networkupdates or disruptions can be added as a second type of sensor in future (see pc-coholic/PyMVGLive#10)

@DavidMStraub
Copy link
Contributor

OK. I am fine with the changes - I leave it to @fabaff whether this can be merged in time for 0.42 or otherwise 78b5eb7 should be removed from the 0.42 PR and postponed to 0.43.

@DavidMStraub
Copy link
Contributor

OK so the original version was released as part of 0.42 now. @mountainsandcode, I suggest you (I can do it as well) open an issue and a PR to resolve the bug of the missing implementation of line restrictions. Then as a second step you can add the improvements for 0.43. OK?

@mountainsandcode
Copy link
Contributor Author

@DavidMStraub I'm travelling for the week without access to my dev machine, so if you have time for the line restriction part then that would be super! Will get to it next week otherwise

@DavidMStraub
Copy link
Contributor

Hi, I added an issue and PR just adding the line name restriction for now.

@mountainsandcode
Copy link
Contributor Author

@DavidMStraub cheers for the work on the line restrictions! I've updated this PR to adress the resulting merge conflict from that bug fix with the changes proposed in PR.
@fabaff, from my side this would be good to merge. Do you have any issues you'd like to see adressed before this can be released?

@balloob balloob merged commit 920d298 into home-assistant:dev Apr 20, 2017
@balloob balloob mentioned this pull request Apr 21, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
@ghost ghost added the integration: mvglive label Mar 21, 2019
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
0