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

Required IdType fixes #471

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Required IdType fixes #471

wants to merge 34 commits into from

Conversation

skinkie
Copy link
Contributor

@skinkie skinkie commented Jul 26, 2023

This pull request tries to address the missing required "id" attributes. This is selected by:

  • GeneralFrameMembers (they must have id, no question)
  • Keyrefs that are already modelled in the NeTEx_publication file

I may have added too many, which I certainly will review. For the following objects there are IdTypes defined, but there is no individual id attribute modelled, this should still be done.

CustomerService
Entrance
FareContractEntry_
GeneralFrameMember
OtherOrganisation
PassengerCarryingRequirementsView
PassingTimeView
Point
ServiceAccessRight_
SignEquipment
TravelSpecification_
TypeOfEntity
VehicleMeetingPlace_
Zone

PassingTimeView and PassengerCarryingRequirementsView are likely a bug in the schema that introduces them in the GeneralFrame. For PassingTimeView a DataManagedObjectView is first introduced, and never used after. For PassengerCarryingRequirementsView in my guess invalidly a DataManagedObject is used (see: #474)

@skinkie skinkie added the enhancement non semantic enhacement: technical enhancement, etc. label Jul 26, 2023
@skinkie skinkie self-assigned this Jul 26, 2023
@skinkie
Copy link
Contributor Author

skinkie commented Jul 26, 2023

Failing examples must be fixed. Already checked with the cardinality in the documentation.

</xsd:annotation>
<!-- TODO: Matthias
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding all this stuff is no good idea. It is already there in Entrance before. That is why the ambiguity occurs.
An id is also there:
image

Is the problem that it is not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What we try to achieve here is that the attribute id gets overridden later with a specific ObjectIdType for that specific element. Theoretically we could add a regular expression then that would guard that the object is named like "codespace:ScheduledStopPoint:something". Not my intention now, but building the relationship between Ref and Id is.

@@ -483,7 +483,7 @@ Rail transport, Roads and Road transport
</xsd:extension>
</xsd:complexContent>
</xsd:complexType>
<xsd:element name="VehicleEntrance" abstract="true" substitutionGroup="Entrance">
<xsd:element name="VehicleEntrance" abstract="true" substitutionGroup="SiteComponent">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This kills the method resolution order (MRO) of the schema. Hence VehicleEntrance will be at the same level as Entrance, while what we want is having the inheritance.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have a problem already, because Entrance comes from SITE and VehicleEntrance probably should not be a SITE. Or do I read this wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

It also might be one of the cases where we need an Entrance_ . Because having to restrictions in a row is not allowed in the inheritance. Should I try to model an Entrance_? According to one of the examples?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we should do the inheritance through the VersionStructure elements. I think that only in the "leaves" the id stuff can be done once.

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 think this is a case where Entrance_ would make sense. In this situation I also want to figure out why (or not) Entrance_ may (or may not) be abstract.

@ue71603
Copy link
Contributor

ue71603 commented Jul 29, 2023

fixed all assigned tasks

@@ -37,7 +37,7 @@
<xsd:element name="points" minOccurs="0">
<xsd:complexType>
<xsd:sequence>
<xsd:element ref="Point" maxOccurs="unbounded"/>
<xsd:element ref="Point_" maxOccurs="unbounded"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure about these changes? It feels like you are referencing to 'abstract' datatypes / 'containers'.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am an engineer (not true): It works... And it seems to me that they did it that way on other xxx_ elements. What it does it references the "concrete" element that is used in the substitutionGroup. Otherwise it wouldn't work. But to be sure we would need a comment from Nick or Christophe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit different from the topic of the PR ;-) but technically should be Ok: I think that this is replacing a ref to anything inheriting from Point to anything being of in the Point_ substitution group. I didn't check the possible semantic impact (yet).

@Aurige
Copy link
Contributor

Aurige commented Sep 6, 2023

I'm not sure that we can be so systematic ! A commented in other PR, I agree that any standalone object that can be referenced should be enforced to have an Id, but when it's an object that is embedded in the XML hierarchy and that will never be referenced, this constraint does not really makes sense (also it is harmless ... except that it brakes existing data).
For example, take the very first example you had to update : the <LimitingRuleInContext> is embedded, and giving it an Id is a constraint that is useless.
I will add this as a discussion point fir next meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement non semantic enhacement: technical enhancement, etc. late_work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants