Skip to content
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

feat!: re-structure state and attributes #7

Merged
merged 19 commits into from
Feb 5, 2024
Merged

Conversation

akloeckner
Copy link
Owner

@akloeckner akloeckner commented Jan 21, 2024

BREAKING CHANGE:

This changes the state to be the actual timestamp of the next non-cancelled departure including delay. This allows to use the state programmatically, e.g. to compute a timedelta from it. Also, the sensor will be shown natively as a relative time, such as "in 5 minutes".

This also changes the attributes to allow for a list of connections to be returned. The following attributes are provided:

  • "next": timestamp of next (i.e. second) departure including delay
  • "next_on": timestamp of over-next (i.e. third) departure including delay
  • "connections": all connections retrieved (max 3 currently)
  • plus all information from the first connection, except the list of legs

Where a connection provides the following information:

  • "origin": name of origin station
  • "departure": timestamp of planned departure
  • "delay": timedelta of departure delay
  • "destination": name of destination station
  • "arrival": timestamp of planned arrival
  • "delay_arrival": timedelta of arrival delay
  • "transfers": number of legs minus one
  • "duration": timedelta departure to arrival
  • "canceled": if any leg is cancelled
  • "ontime": if no departure delay
  • "products": comma-separated list of line names
  • "legs": list of legs with more detailed information

Where each leg contains the following information:

  • "origin": name of origin station
  • "departure": timestamp of planned departure
  • "platform": departure platform
  • "delay": timedelta of departure delay
  • "destination": name of destination station
  • "arrival": timestamp of planned arrival
  • "platform_arrival": arrival platform
  • "delay_arrival": timedelta of arrival delay
  • "mode": transport mode such as train
  • "name": name of transport such as RE123
  • "canceled": Boolean
  • "distance": walking distance if any
  • "remarks": list of strings
  • "stopovers": list of station names

Related: #4

@akloeckner
Copy link
Owner Author

@risiko79, @sc-ampersand-p, @kilimnik

What do you think? I have this running on my instance for some time. It works well. However, I might miss some use cases, as I am not a heavy user.

I am actually converting the data to the old state format for display on my wall-panel. However, I still think it is cleaner to use an actual timestamp as state.

@sc-ampersand-p
Copy link

@akloeckner I think this sounds pretty solid, I can't think of any usecase that requires any additional information.

Using a timestamp seems the most logical and robust path forward.

@pinsdorf
Copy link

pinsdorf commented Jan 22, 2024

Allow an observer of this effort to comment on this. I like the datastructure. It covers all the use case I can think of from top of my hat. Also the datetime proposal sounds great.

However, I'd advise not to have keys next and next_on. This limits us to 3 connections and is hard to change later. I'd rather suggest a list of timestamps of upcoming connections under next like this:

next:
  - timestamp of connection 1 
  - timestamp of connection 2 
  - timestamp of connection 3 
  - ...

This would allow for a later extension of the list to a user-defined length.

Copy link

@pinsdorf pinsdorf left a comment

Choose a reason for hiding this comment

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

Looks good to me, but as external observer I'd abstain from giving a formal review vote here.

custom_components/hafas/sensor.py Show resolved Hide resolved
custom_components/hafas/sensor.py Outdated Show resolved Hide resolved
custom_components/hafas/sensor.py Outdated Show resolved Hide resolved
custom_components/hafas/sensor.py Outdated Show resolved Hide resolved
@akloeckner
Copy link
Owner Author

However, I'd advise not to have keys next and next_on. This limits us to 3 connections and is hard to change later. I'd rather suggest a list of timestamps of upcoming connections under next

Good suggestion! I just factored it in, because it was there before. But: We're leaving the "compatible" path anyways, so why not also change that... To be radical, I would then even not include this list at all, because it is there already in the connections attribute. What do you think?

We might argue to rename it to next, though.

@pinsdorf
Copy link

However, I'd advise not to have keys next and next_on. This limits us to 3 connections and is hard to change later. I'd rather suggest a list of timestamps of upcoming connections under next

Good suggestion! I just factored it in, because it was there before. But: We're leaving the "compatible" path anyways, so why not also change that... To be radical, I would then even not include this list at all, because it is there already in the connections attribute. What do you think?

We might argue to rename it to next, though.

Good point. How about next_arrivals, next_connections, or next_opprtunities? I'd prefer next_arrivals as this hints at timestamps. next_connectionssound more like a list of elements from a more complex data structure.

@akloeckner akloeckner mentioned this pull request Jan 24, 2024
@akloeckner
Copy link
Owner Author

akloeckner commented Jan 24, 2024

Good point. How about next_arrivals, next_connections, or next_opprtunities?

Looking at the other integrations again - see #4 (comment) - I think we should use connections or departures. I'd not use next_*, because the list will contain also the current departure, which might cause confusion. And I'd also not use *_arrivals, because the main state is not the arrival timestamp (at the destination), but the departure timestamp (from the origin).

Now, as the connections returned are actually a complex data structure, I think, connections is the best name for it.

However: we would not have a list of pure timestamps then. Just the complex data structure. Which I don't consider a problem, because we can generate a list of timestamps easily in a Jinja2 template:

{{ state_attr('sensor.koln_hbf_to_frankfurt_main_hbf', 'connections')
   | map(attribute='departure')
   | list }}

Edit: Template for all non-canceled connections:

{{ state_attr('sensor.koln_rath_heumar_porzer_str_to_koln_rath_heumar_konigsforst', 'connections')
   | rejectattr('canceled')
   | map(attribute='departure')
   | list }}

@akloeckner
Copy link
Owner Author

I'd be fine to merge this.

Thank you, @pinsdorf, for your helpful feedback!

@kilimnik, any last thoughts? I don't want to mess up your work.

@akloeckner akloeckner merged commit 5c33cf2 into master Feb 5, 2024
5 checks passed
@akloeckner
Copy link
Owner Author

For reference, a change like this was also introduced in the swiss public transport official integration: home-assistant/core#106485

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants