akloeckner / hacs-hafas

Port of kilimnik's HAFAS client for HACS
4 stars 1 forks source link

feat!: re-structure state and attributes #7

Closed akloeckner closed 5 months ago

akloeckner commented 6 months ago

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:

Where a connection provides the following information:

Where each leg contains the following information:

Related: #4

akloeckner commented 6 months ago

@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 commented 6 months ago

@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 commented 5 months ago

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.

akloeckner commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

Looking at the other integrations again - see https://github.com/akloeckner/hacs-hafas/issues/4#issuecomment-1688888130 - 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 commented 5 months ago

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 commented 4 months ago

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