Fix AccessEquipmentRef by skinkie · Pull Request #168 · TransmodelEcosystem/NeTEx

skinkie · 2021-04-25T16:15:03Z

(see issue, for documentation)

Aurige · 2021-04-26T15:34:56Z

I approved this fix since it makes sense, but it may have some consequences on existing datasets (but quite unlikely since Equipment or not widely used yet)

syversenkr · 2021-04-26T17:58:47Z

Hopefully I am not missing something here, but from what I can see in the XSD this bug fix will only allow for references previously not part of the placeEquipments refenece substitution (InstalledEquipmentRef), i.e. the AccessEquipmentRef subtypes. The element types themselves where already correctly of InstalledEquipment substitution type.

On the other hand: In order to align with the UML/spec, it seems these references should more appropriately have been of a PlaceEquipment(Ref) substitution group.

syversenkr · 2021-04-26T18:01:03Z

On the other hand, both the InstalledEquipment type/ref and PlaceEquipment type/ref are abstract conceptual classes of super Equipment type/ref, and this has no practical implication other than adding an extra level of dummy element/reference in the XSD.

skinkie · 2021-04-26T18:07:33Z

@syversenkr I did not find a PlaceEquipmentStructure did you?,

syversenkr · 2021-04-26T18:09:33Z

No, it is not yet added. A technical shortcut when implementing, I presume.

I believe there are similar technical simplifications elsewhere in the code, if we want to reflect this particular additional layer I can add it to the branch.

skinkie · 2021-04-26T18:14:30Z

No, it was not yet added. A technical shortcut when implementing, I presume.

I can add if we want to reflect this additional layer.

I am checking SiteEquipmentRef and SignEquipmentRef which have exactly the same problem.

<xsd:element name="SiteEquipmentRef" type="SiteEquipmentRefStructure" substitutionGroup="EquipmentRef">
<xsd:element name="SignEquipmentRef" type="EquipmentRefStructure" substitutionGroup="EquipmentRef">

So the question is, should we create the PlaceEquipment as substitution?

…btypes can)

syversenkr · 2021-04-26T20:11:12Z

Commit 047c548:
Should be specified as abstract, as its implementation type AccessEquipment_VersionStructure is abstract, and adding AccessEquipment as an explicit element in a list of AccessEquipment type elements is already inherently invalid.

PlaceEquipment substitution type:
Both results in the same XML, so no technical impact if wrapping in one or two abstract classes. More a discussion if worth the effort, i.e. should the XSD implement the spec technically or as close to the model as possible.

However, the additional incorrect references with substitution group Equipment should be fixed as part of this commit.

skinkie · 2021-04-26T20:14:12Z

 	</xsd:group>
 	<!-- ====ACCESS=================================================================== -->
-	<xsd:element name="AccessEquipment" type="AccessEquipment_VersionStructure" substitutionGroup="InstalledEquipment">
+	<xsd:element name="AccessEquipment" type="AccessEquipment_VersionStructure" abstract="true" substitutionGroup="InstalledEquipment">


What does this 'abstract' mean? "Optional. Specifies whether the element can be used in an instance document. True indicates that the element cannot appear in the instance document. Instead, another element whose substitutionGroup attribute contains the qualified name (QName) of this element must appear in this element's place. Default is false"

see comment above
This is inherently abstract per type implementation, practical implication is that currently you can add AccessEquipment as a list element (and it is proposed as a code completion option in e.g. oXygen and XMLspy), but if you do add an explicit AccessEquipment element rather than one of its implementable subtypes you get a validation error.

Before fix:

Adding as list element => Validation error: "The type definition cannot be abstract for element AccessEquipment."

skinkie · 2021-04-26T20:16:44Z

@nick-knowles can you comment on the question @syversenkr raises?

syversenkr · 2021-04-26T20:29:08Z

The question being: Should we create the PlaceEquipment as substitution to align with the UML model? Or keep the simpler implementation, skipping abstract types when it does not have a practical implication?

Aurige · 2021-04-26T20:50:45Z

The question being: Should we create the PlaceEquipment as substitution to align with the UML model? Or keep the simpler implementation, skipping abstract types when it does not have a practical implication?

I would keep it simple

skinkie · 2021-05-12T13:24:53Z

I think the only practical implementation this will have is if we are to be generating UML diagrams from the XSD, it must be modeled.

skinkie · 2021-05-21T15:47:13Z

I suggest we are going to do PlantUML, hence do it via the long way.

nick-knowles · 2021-07-13T23:54:51Z

The AccessEquipment should also be abstract (not just the AccessEquipmentRef)

nick-knowles · 2021-08-26T15:07:14Z

XML has some almost unfathomable extra restrictions on type substitution, so it is not always possible to simply encode the UML.

ue71603 · 2022-01-04T14:04:36Z

@skinkie This is still blocked?

skinkie · 2022-01-04T14:07:03Z

@skinkie This is still blocked?

Not blocked, but a suggestion what a solution would be, would be appreciated. I am happy to add abstract=true to all the blue classes.

ue71603 · 2022-01-04T14:07:57Z

@Aurige @nick-knowles : Could you give your opinion to Stefan?

Aurige · 2022-01-04T17:23:47Z

yes according to the UML they look abstract

ue71603 · 2022-04-14T11:59:54Z

 			<xsd:documentation>Type for a reference to an ACCESS EQUIPMENT.</xsd:documentation>
 		</xsd:annotation>
 		<xsd:simpleContent>
-			<xsd:restriction base="EquipmentRefStructure">


This is done in #293

ue71603 · 2022-04-14T11:59:54Z

 			<xsd:documentation>Type for a reference to an ACCESS EQUIPMENT.</xsd:documentation>
 		</xsd:annotation>
 		<xsd:simpleContent>
-			<xsd:restriction base="EquipmentRefStructure">


This is done in #293

ue71603 · 2022-04-14T12:06:44Z

 		</xsd:sequence>
 	</xsd:group>
 	<!-- ====ACCESS=================================================================== -->
-	<xsd:element name="AccessEquipment" type="AccessEquipment_VersionStructure" substitutionGroup="InstalledEquipment">


already done in master

ue71603 · 2022-04-14T12:06:44Z

 		</xsd:sequence>
 	</xsd:group>
 	<!-- ====ACCESS=================================================================== -->
-	<xsd:element name="AccessEquipment" type="AccessEquipment_VersionStructure" substitutionGroup="InstalledEquipment">


already done in master

ue71603 · 2022-04-14T12:07:45Z

+			<xsd:restriction base="InstalledEquipmentRefStructure">
 				<xsd:attribute name="ref" type="AccessEquipmentIdType" use="required">
 					<xsd:annotation>
-						<xsd:documentation>Identifier of a TICKETING EQUIPMENT.</xsd:documentation>


wrong in master. This some documentation change should be done.

ue71603 · 2022-04-14T12:07:45Z

+			<xsd:restriction base="InstalledEquipmentRefStructure">
 				<xsd:attribute name="ref" type="AccessEquipmentIdType" use="required">
 					<xsd:annotation>
-						<xsd:documentation>Identifier of a TICKETING EQUIPMENT.</xsd:documentation>


wrong in master. This some documentation change should be done.

ue71603 · 2022-04-14T12:09:01Z

 		</xsd:annotation>
 		<xsd:restriction base="InstalledEquipmentIdType"/>
 	</xsd:simpleType>
-	<xsd:element name="AccessEquipmentRef" type="AccessEquipmentRefStructure" substitutionGroup="EquipmentRef">


This is new. It currently works with EquipmentRef (as this is the more generalized class). However, to be very in line, whe should probably keep this change. How pedantic do we want to be.

ue71603 · 2022-04-14T12:09:01Z

 		</xsd:annotation>
 		<xsd:restriction base="InstalledEquipmentIdType"/>
 	</xsd:simpleType>
-	<xsd:element name="AccessEquipmentRef" type="AccessEquipmentRefStructure" substitutionGroup="EquipmentRef">


This is new. It currently works with EquipmentRef (as this is the more generalized class). However, to be very in line, whe should probably keep this change. How pedantic do we want to be.

https://github.com/NeTEx-CEN/NeTEx/pull/293/files contains an example. Therefore it should break master, if we have a problem there.

ue71603

Changing <xsd:element name="AccessEquipmentRef" type="AccessEquipmentRefStructure" substitutionGroup="EquipmentRef"> we should consider. Correcting the documentation TICKETING to ACCESS. The rest should be thrown out.

Already fixed

* Fix AccessEquipmentRef Fix TransmodelEcosystem#167 * Should be abstract as this element cannot be implemented (only its subtypes can) * Honest attempt to change the model to the UML description. Co-authored-by: Kristian Syversen <kristian.syversen@entur.org>

Fix AccessEquipmentRef

1635921

Fix #167

skinkie requested review from Aurige, nick-knowles and syversenkr April 25, 2021 16:15

skinkie assigned Aurige Apr 25, 2021

skinkie added enhancement non semantic enhacement: technical enhancement, etc. bug Technical mistake, inconsistency with the documentation, etc. and removed enhancement non semantic enhacement: technical enhancement, etc. labels Apr 25, 2021

Aurige approved these changes Apr 26, 2021

View reviewed changes

syversenkr approved these changes Apr 26, 2021 •

edited

Loading

View reviewed changes

Should be abstract as this element cannot be implemented (only its su…

047c548

…btypes can)

skinkie commented Apr 26, 2021

View reviewed changes

skinkie marked this pull request as draft April 26, 2021 20:16

skinkie assigned nick-knowles Apr 26, 2021

nick-knowles approved these changes Aug 26, 2021

View reviewed changes

skinkie mentioned this pull request Apr 13, 2022

[FIX] AccessEquipmentRef incorrently subsitutes EquipmentRef #167

Closed

ue71603 reviewed Apr 14, 2022

View reviewed changes

ue71603 previously requested changes Apr 14, 2022

View reviewed changes

ue71603 assigned skinkie and unassigned nick-knowles and Aurige Apr 14, 2022

skinkie added 3 commits April 15, 2022 22:59

Honest attempt to change the model to the UML description.

e46c975

Extra fixes

4986b91

Merge branch 'master' into fix_access_equipmentref

135de94

skinkie marked this pull request as ready for review April 15, 2022 21:08

skinkie requested a review from ue71603 April 15, 2022 21:15

skinkie merged commit 45162f2 into master Apr 15, 2022

skinkie deleted the fix_access_equipmentref branch November 19, 2024 10:27

Conversation

skinkie commented Apr 25, 2021

Uh oh!

Aurige commented Apr 26, 2021

Uh oh!

syversenkr commented Apr 26, 2021

Uh oh!

syversenkr commented Apr 26, 2021

Uh oh!

Uh oh!

skinkie commented Apr 26, 2021

Uh oh!

syversenkr commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skinkie commented Apr 26, 2021

Uh oh!

syversenkr commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skinkie Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syversenkr Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syversenkr Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinkie commented Apr 26, 2021

Uh oh!

syversenkr commented Apr 26, 2021

Uh oh!

Aurige commented Apr 26, 2021

Uh oh!

skinkie commented May 12, 2021

Uh oh!

skinkie commented May 21, 2021

Uh oh!

nick-knowles commented Jul 13, 2021

Uh oh!

nick-knowles commented Aug 26, 2021

Uh oh!

ue71603 commented Jan 4, 2022

Uh oh!

skinkie commented Jan 4, 2022

Uh oh!

ue71603 commented Jan 4, 2022

Uh oh!

Aurige commented Jan 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ue71603 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

syversenkr commented Apr 26, 2021 •

edited

Loading

syversenkr commented Apr 26, 2021 •

edited

Loading

skinkie Apr 26, 2021 •

edited

Loading

syversenkr Apr 26, 2021 •

edited

Loading

syversenkr Apr 26, 2021 •

edited

Loading