-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Deprecate WallID as direct child under Side #345
Conversation
See proposal
BuildingSync.xsd
Outdated
@@ -1179,7 +1179,6 @@ | |||
</xs:complexType> | |||
</xs:element> | |||
<xs:choice> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need the choice
element to avoid breaking change to Audit Template? Per this comment: #223 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove xs:choice
, Mark's comment was saying he preferred that users could choose between WallID or WallIDs. But we are removing that choice completely now, so the xs:choice
element is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense. I removed xs:choice
.
Another issue is that deleting the choice of directly using WallID causes validation failures to all examples, as they all don't use WallIDs
.
BuildingSync.xsd
Outdated
@@ -1179,7 +1179,6 @@ | |||
</xs:complexType> | |||
</xs:element> | |||
<xs:choice> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove xs:choice
, Mark's comment was saying he preferred that users could choose between WallID or WallIDs. But we are removing that choice completely now, so the xs:choice
element is no longer needed.
proposals/2021/Deprecate WallID.md
Outdated
<xs:complexType> | ||
<xs:sequence> | ||
... | ||
<xs:choice> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks like we need to remove choice here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I just updated the proposal.
`WallIDs` was added as an `xsd:choice` element in [#223](https://github.com/BuildingSync/schema/pull/223) to allow multiple `auc:WallID`s referenced for a `auc:Side` element (unbounded). This made the single `auc:WallID` reference choice redundant, so we propose to remove it. | ||
On the other hand, to avoid breaking change for Audit Template, the `xsd:choice` element will remain. | ||
`auc:WallIDs` was added as an `xsd:choice` element in [#223](https://github.com/BuildingSync/schema/pull/223) to allow multiple `auc:WallID`s referenced for a `auc:Side` element (unbounded). This made the single `auc:WallID` reference choice redundant, so we propose to remove it. | ||
In addition, as `auc:WallIDs` becomes the only choice, the `xsd:choice` element is no longer needed, so we propose to remove this layer and make `auc:WallIDs` direct child under `auc:Side`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 👍
@JieXiong9119 Are you planning on updating the examples so these tests will pass? Or are we waiting for @macintoshpie 's translator? |
I'll probably update the examples first. |
BuildingSync.xsd
Outdated
</xs:complexType> | ||
</xs:element> | ||
</xs:choice> | ||
<xs:element name="WallIDs"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<xs:element name="WallIDs"> | |
<xs:element name="WallIDs" minOccurs="0" maxOccurs="1"> |
@JieXiong9119 can you resolve these conflicts? Once that's done we should be good to merge |
Just did. Could you check one more time and merge it if it looks good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
See proposal.
Resolves #233