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

Add AccessFeatureStatus on PathLink #238

Merged
merged 22 commits into from
Jan 20, 2023

Conversation

ue71603
Copy link
Contributor

@ue71603 ue71603 commented Aug 26, 2022

AccessFeatureStatus on a PathLink to indicate a broken access equipment.

#194 asks for data to tell whether, e.g., an elevator is broken, but to nevertheless show the route so that the passenger knows that this route would exist but is currently not feasible.

EquipmentStatusEnumeration has been added from NeTEx to indicate the status of an access equipment. The following would indicate that the elevator is broken:

PathLink.AccessFeatureStatus: notAvailable (new)
PathLink.AccessFeatureType: elevator

The broken elevator makes the complete TransferLeg unfeasible - this should be flagged in the TransferLeg (e.g., TransferLeg.Feasibility : accessEquipmentOutOfService) or/and at the level of the Trip.
What solution would you prefer?

To also handle the case that the TranferLeg is not feasible due to a SituationFull (can that happen?), this could be indicated by:
TransferLeg.Feasibility: seeSituations

Replaces #235

Aurige
Aurige previously approved these changes Aug 26, 2022
@Aurige
Copy link
Contributor

Aurige commented Aug 26, 2022

I approved it since I'm ok with it, but won't be against a "partiallyAvailable" if somebody thinks that it is useful (double door with one broken, etc.) ... but I recognise that it is often difficult to use

@trurlurl
Copy link
Contributor

trurlurl commented Sep 2, 2022

I think the questions raised in the first comment should be answered before closing this PR - what are your opinions regarding
TransferLeg.Feasibility : accessEquipmentOutOfService / seeSituations or
Trip.Feasibility ?

@trurlurl
Copy link
Contributor

Documentation regarding the PathLink part is up do date so far.

@ue71603
Copy link
Contributor Author

ue71603 commented Oct 18, 2022

Will discussed the 25.10.
Malte: Important: How to request it. I would not do this normally. The result structure is fine. The point from André and overall request must be discussed.
Christophe: Partial only in connection with a comment.
Malte: Depends, if one wants to show such a journey.
We would need a notice on the element as well.
==> Matthias: Checks the partial part.

@ue71603
Copy link
Contributor Author

ue71603 commented Oct 18, 2022

André: Can the non-working in the situation?
Stefan: This works. When I am aggregator, how do I communicate if it is still available..
André: AccessFeature and the Situation should be linked.

Copy link
Contributor

@sgrossberndt sgrossberndt left a comment

Choose a reason for hiding this comment

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

SIRI contains

  • an EquipmentStatusEnumeration in siri/ifopt/ifopt_equipment-v0.3.xsd with values unknown, available, notAvailable
  • a FacilityStatusEnumeration in siri/siri_model/siri_facility-v2.0.xsd with values unknown, available, notAvailable, partiallyAvailable, added, removed

As both don't completely fit I guess its fine to introduce our own enumeration. Or should we use one from SIRI? If we define our own enumeration - is "EQUIPMENT" the correct term here or should it rather be AccessFeatureStatusEnumeration?

@trurlurl
Copy link
Contributor

@sgrossberndt AccessFeatureStatusEnumeration would be nicer when looking at OJP alone, EquipmentStatusEnumeration was chosen since it refers to the NeTEx term. I'm changing it to AccessFeatureStatusEnumeration.

@trurlurl
Copy link
Contributor

I hesitated adding a AccessFeatureRef (siri/ifopt/EquipmentRefStructure). I didn't add it yet because the situation information relating to the Equipment can be directly attached to the PathLink (SituationFullRef).

@ue71603
Copy link
Contributor Author

ue71603 commented Oct 31, 2022

André is doing another small change, then ready for review.

Aurige
Aurige previously approved these changes Oct 31, 2022
trurlurl
trurlurl previously approved these changes Nov 24, 2022
skinkie
skinkie previously approved these changes Nov 24, 2022
@ue71603 ue71603 dismissed stale reviews from skinkie, trurlurl, and Aurige via 7c2b429 November 26, 2022 13:26
@ue71603 ue71603 force-pushed the AddAccessFeatureStatusOnPathLink branch from bcc7485 to 5e15719 Compare December 17, 2022 11:48
OJP/OJP_Trips.xsd Show resolved Hide resolved
@ue71603 ue71603 dismissed stale reviews from AndreasAtSBB and trurlurl via e51fbf8 January 20, 2023 10:54
@ue71603 ue71603 force-pushed the AddAccessFeatureStatusOnPathLink branch from f4886eb to e51fbf8 Compare January 20, 2023 10:54
Copy link

@AndreasAtSBB AndreasAtSBB left a comment

Choose a reason for hiding this comment

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

OK.

@skinkie skinkie merged commit 59406bb into changes_for_v1.1 Jan 20, 2023
@skinkie skinkie deleted the AddAccessFeatureStatusOnPathLink branch January 20, 2023 11:11
@@ -471,12 +476,17 @@
</xs:annotation>
</xs:element>
<xs:group ref="OperatingDaysGroup" minOccurs="0"/>
<xs:group ref="TripStatusGroup" minOccurs="0"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This merge moved the position of TripStatusGroup. Was this on purpose?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please report all collateral damage due to manual merging to @ue71603

Copy link
Contributor

Choose a reason for hiding this comment

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

@sgrossberndt and the fact that this change was not picked up by CI shows we lack examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it on purpose. In all other examples Situations were the last before Extension. It thought it was a mistake during the change in Situations. Certainly I should not have done it in this PR if that was not the case (as it seems), but from homogenity, I think it is right. I can undo it in an additional PR if you like @sgrossberndt

Copy link
Contributor

Choose a reason for hiding this comment

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

No, don't undo it. But it should not have been part of this PR but handled separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know... I only did it, because I thought it was messed up during the merge by me or during the creation of the PR. Should have checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc updated enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants