Update NSAPI to version 3.0.0 by YarmoM · Pull Request #30599 · home-assistant/core

YarmoM · 2020-01-08T21:38:41Z

Breaking Changes:

The nederlandse_spoorwegen sensor now requires a single API token instead of username and password credentials.

The following platform attributes are renamed for consistency with time naming:

departure_platform becomes departure_platform_planned
departure_platform_changed becomes departure_platform_actual
arrival_platform becomes arrival_platform_planned
arrival_platform_changed becomes arrival_platform_actual

Description:

The Nederlandse Spoorwegen API has recently changed and so has nsapi, the dependency home-assistant uses to communice with said API. This PR bumps the nsapi version to 3.0.0 and fixes some of its breaking changes.

Related issue (if applicable): fixes #26622

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

Example entry for `configuration.yaml` (if applicable):

- platform: nederlandse_spoorwegen
  password: !secret ns_api_key
  routes:
  - name: Rijswijk-Rotterdam
    from: Rsw
    to: Rtd

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.
I have followed the development checklist

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

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

homeassistant · 2020-01-08T21:38:43Z

Hi @YarmoM,

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!

YarmoM · 2020-01-08T21:42:46Z

I have updated both manifest file and requirements_all, however running either python3 -m script.hassfest or python3 -m script.gen_requirements_all results in a syntax error. Is this step something that can be done by a reviewer? The code works locally.

frenck · 2020-01-09T09:47:11Z

Could you elaborate a bit on the syntax error you've received?

fabaff · 2020-01-09T10:16:26Z

According to the CI is black not passing, seems like a formatting issue.

$ black homeassistant/components/nederlandse_spoorwegen/sensor.py

should do the job.

YarmoM · 2020-01-09T11:09:46Z

According to the CI is black not passing, seems like a formatting issue.
$ black homeassistant/components/nederlandse_spoorwegen/sensor.py
should do the job.

Ran the command, it says:
All done!
1 file left unchanged

Indeed, the file has not been modified by black.

YarmoM · 2020-01-09T11:13:06Z

Could you elaborate a bit on the syntax error you've received?

When running script.hassfest, it says:

File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/yarmo/temp/home-assistant/script/hassfest/__main__.py", line 5, in <module>
    from . import (
  File "/home/yarmo/temp/home-assistant/script/hassfest/dependencies.py", line 6, in <module>
    from homeassistant.requirements import DISCOVERY_INTEGRATIONS
  File "/home/yarmo/temp/home-assistant/homeassistant/requirements.py", line 8, in <module>
    from homeassistant.core import HomeAssistant
  File "/home/yarmo/temp/home-assistant/homeassistant/core.py", line 33, in <module>
    from async_timeout import timeout
ModuleNotFoundError: No module named 'async_timeout'

For script.gen_requirements_all:

File "/usr/lib/python3.7/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/yarmo/temp/home-assistant/script/gen_requirements_all.py", line 13, in <module>
    from homeassistant.util.yaml.loader import load_yaml
  File "/home/yarmo/temp/home-assistant/homeassistant/util/__init__.py", line 23, in <module>
    import slugify as unicode_slug
ModuleNotFoundError: No module named 'slugify'

Python is not my forte, I'm trying to solve it

frenck · 2020-01-09T11:31:25Z

@YarmoM Looks like you haven't set up your development environment correctly since it is missing dependencies. For more information on how to set up a local development environment, see: https://developers.home-assistant.io/docs/en/development_environment.html

YarmoM · 2020-01-09T11:32:06Z

Did the "Set up Development Environment" again, noticed I forgot to run script/setup... :(

Ran both scripts, first one returned zero invalid integrations and second ran without output.

--EDIT---
Just noticed your previous comment, yes that was it

YarmoM · 2020-01-09T11:38:18Z

CI says to run pre-commit run --all-files to reproduce the error locally, trying that now.

---UPDATE---
The whole thing passed! Somehow the CI was apparently bugging out... The CI asked me to run a command to reproduce the error, and it's passing!

Can we ask the CI to run again?

---UPDATE---
Removed the whole development folder, started again from scratch, black fails. Alrighty, I've got work to do...

YarmoM · 2020-01-09T12:01:26Z

Ok, this is getting strange.

So I git cloned all over again, checkout branch, git pull.

I run pre-commit run --all-files and black fails.

I run black homeassistant/components/nederlandse_spoorwegen/sensor.py and it returns 1 file left unchanged.

I run pre-commit run --all-files again and black passes.

I'm not sure how to solve this.

---UPDATE---
Checked git status and the file DID get modified. So black did its thing and somehow returned 1 file left unchanged. Strange behavior but problem solved.

amelchio

Looks good, let me just propose a couple of cleanups.

Also, this seems like a breaking change so you should fill out that section of the PR template.

YarmoM · 2020-01-10T08:24:25Z

Agreed on all accounts. This PR started as a simple fix to get it working again as soon as possible, but the more I look at the code, the more it begs for some improvements. Alright, let's do this the correct way.

Question: how do you feel about the same sensor being able to produce different outputs? This sensor currently reports on the next train departure. This should always happens when you input a departure station and an destination station. Now, let's say functionality gets added that detects whether you only inputted a departure station. The code notices this and goes into "disruption mode", giving feedback on whether something has happened at that station.

Technically, this is easy, but it does not follow the "1 component 1 function" principle. How do you feel about this? Or should this be a separate sensor?

amelchio · 2020-01-10T20:32:19Z

+            attributes["departure_time_actual"] = self._trips[
+                0
+            ].departure_time_planned.strftime("%H:%M")


Why do you put departure_time_planned into departure_time_actual? It is already in departure_time_planned so I don't see that you are adding any new information, only duplication.

This made most sense to me, mainly 2 reasons:

it is named "actual" so we need to get the actual departure time, this could either be the planned time or a delayed time but in all cases should the "actual" time be the time the train will depart;

this is a conditional statement that needs to be made at some point. If we set "actual" to None, later you must first check if "actual" is none, if so take "planned", if not take "actual".

I would prefer to not go back entirely to the old way so I propose two solutions:

we keep two departure times (requiring a conditional later on) but then "actual" needs to be refactored because that name is misleading, or

we switch to a single departure time, there's already a "delayed" Boolean so that gives you most of the info needed.

Or third solution, we keep this. "Planned" is a reference time, "actual" is the actual time whether planned or delayed.

Fixes and enhancements should not go into the same PR. Please limit the attribute changes to those required for the API update (if any).

It is fine to submit further PRs with the remaining proposals.

Will do, thanks.

amelchio · 2020-01-10T20:32:49Z

+            attributes["arrival_time_actual"] = self._trips[
+                0
+            ].arrival_time_planned.strftime("%H:%M")


amelchio · 2020-01-11T20:36:29Z

Sorry for being a bit terse. I have had only little time for review the last couple of days and thought you might like a quick response even if it was short.

About your question, the current configuration is in a routes: section. Rather than allowing it to have two forms, I think it will be better to add for example a stations: section for the functionality that you mention. The Python code can still be in the same module and share some methods.

YarmoM · 2020-01-11T23:07:21Z

Sorry for being a bit terse. I have had only little time for review the last couple of days and thought you might like a quick response even if it was short.

About your question, the current configuration is in a routes: section. Rather than allowing it to have two forms, I think it will be better to add for example a stations: section for the functionality that you mention. The Python code can still be in the same module and share some methods.

No problemo, sir! Thank you for your time and dedication and I did appreciate a quick reply so that I could get to work! Thanks for the stations: sections, that makes sense, will look into implementing that in the future

amelchio

All right, let's go with this. Thank you for your persistence!

YarmoM · 2020-01-12T08:43:07Z

Thank you again for your time reviewing this PR, your effort is much appreciated!

Updated NSAPI to version 3.0.0

1a69c99

homeassistant added the cla-needed label Jan 8, 2020

probot-home-assistant Bot added integration: nederlandse_spoorwegen small-pr PRs with less than 30 lines. labels Jan 8, 2020

homeassistant added cla-signed and removed cla-needed labels Jan 8, 2020

MartinHjelmare changed the title ~~Updated NSAPI to version 3.0.0~~ Update NSAPI to version 3.0.0 Jan 9, 2020

Fixed code style error

6abb217

YarmoM marked this pull request as ready for review January 9, 2020 12:39

amelchio requested changes Jan 9, 2020

View reviewed changes

Comment thread homeassistant/components/nederlandse_spoorwegen/sensor.py Outdated

Comment thread homeassistant/components/nederlandse_spoorwegen/sensor.py Outdated

amelchio added the breaking-change label Jan 9, 2020

YarmoM added 5 commits January 10, 2020 12:16

Restructured API access

b382a1c

Improved construction of attributes

24ac98e

Fixed handling of API output

0f48f7e

Added @YarmoM as code owner

78c44db

Updated CODEOWNERS

b00f2fe

YarmoM mentioned this pull request Jan 10, 2020

Update nederlandse_spoorwegen.markdown home-assistant/home-assistant.io#11700

Merged

2 tasks

YarmoM requested a review from amelchio January 10, 2020 13:05

amelchio reviewed Jan 10, 2020

View reviewed changes

YarmoM requested a review from amelchio January 11, 2020 12:12

Reverted changes for full backwards compatibility

c6eddb8

amelchio reviewed Jan 11, 2020

View reviewed changes

Comment thread homeassistant/components/nederlandse_spoorwegen/sensor.py

YarmoM added 2 commits January 12, 2020 00:13

Fixed bad conditional

5144270

Simplify conditional

19ba91c

amelchio approved these changes Jan 12, 2020

View reviewed changes

YarmoM mentioned this pull request Jan 12, 2020

Confusing departure_time_actual behavior aquatix/ns-api#19

Closed

amelchio merged commit 96bf8bc into home-assistant:dev Jan 12, 2020

lock Bot locked and limited conversation to collaborators Jan 13, 2020

Uh oh!

Conversation

YarmoM commented Jan 8, 2020 • edited by MartinHjelmare Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Changes:

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

homeassistant commented Jan 8, 2020

Uh oh!

YarmoM commented Jan 8, 2020

Uh oh!

frenck commented Jan 9, 2020

Uh oh!

fabaff commented Jan 9, 2020

Uh oh!

YarmoM commented Jan 9, 2020

Uh oh!

YarmoM commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frenck commented Jan 9, 2020

Uh oh!

YarmoM commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YarmoM commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YarmoM commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

YarmoM commented Jan 10, 2020

Uh oh!

amelchio Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

YarmoM Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

amelchio Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

YarmoM Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

amelchio Jan 10, 2020

Choose a reason for hiding this comment

Uh oh!

amelchio commented Jan 11, 2020

Uh oh!

Uh oh!

YarmoM commented Jan 11, 2020

Uh oh!

amelchio left a comment

Choose a reason for hiding this comment

Uh oh!

YarmoM commented Jan 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

YarmoM commented Jan 8, 2020 •

edited by MartinHjelmare

Loading

Example entry for `configuration.yaml` (if applicable):

YarmoM commented Jan 9, 2020 •

edited

Loading

YarmoM commented Jan 9, 2020 •

edited

Loading

YarmoM commented Jan 9, 2020 •

edited

Loading

YarmoM commented Jan 9, 2020 •

edited

Loading