Skip to content

Add TotalRoofArea #307

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

Merged
merged 3 commits into from
Mar 23, 2021
Merged

Add TotalRoofArea #307

merged 3 commits into from
Mar 23, 2021

Conversation

JieXiong9119
Copy link
Contributor

See proposal
Implementation in schema added

BuildingSync.xsd Outdated
@@ -796,6 +796,18 @@
</xs:simpleContent>
</xs:complexType>
</xs:element>
<xs:element name="TotalExteriorRoofArea" 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.

Wondering if we should use TotalRoofArea instead? I don't have a preference, was wondering if there might have been a reason we added Exterior to the wall area one, but if it is necessary for roofs.

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 total roof area is concise and better. Is there a non-exterior/interior roof anywhere?

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 TotalRoofArea is good.

BuildingSync.xsd Outdated
</xs:annotation>
<xs:complexType>
<xs:simpleContent>
<xs:extension base="xs:decimal">
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe make this as a non negative decimal type (we might need to add this as a base type). see the BoundedDecimalZeroToOne for something similar.

Copy link
Contributor

@corymosiman12 corymosiman12 left a comment

Choose a reason for hiding this comment

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

  • create new simple type for nonNegativeDecimal, something like:
<xs:simpleType name="BoundedDecimalZeroToOne">
    <xs:restriction base="xs:decimal">
      <xs:minInclusive value="0"/>
      <xs:maxInclusive value="1"/>
    </xs:restriction>
  </xs:simpleType>
  • Use TotalRoofArea

Else looks good!

1. Changed `TotalExteriorRoofArea` to `TotalRoofArea`.
2. Created new simple type for `nonNegativeDecimal` and applied for the added element.
@JieXiong9119
Copy link
Contributor Author

  • create new simple type for nonNegativeDecimal, something like:
<xs:simpleType name="BoundedDecimalZeroToOne">
    <xs:restriction base="xs:decimal">
      <xs:minInclusive value="0"/>
      <xs:maxInclusive value="1"/>
    </xs:restriction>
  </xs:simpleType>
  • Use TotalRoofArea

Else looks good!

Done! Please check it again.
Should we rename the PR as well?

@corymosiman12
Copy link
Contributor

Perfect. Yeah, I think renaming would be good.

@JieXiong9119 JieXiong9119 changed the title Proposals/total exterior roof area Proposals/total roof area Mar 19, 2021
Copy link
Contributor

@corymosiman12 corymosiman12 left a comment

Choose a reason for hiding this comment

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

👍

@nllong
Copy link
Member

nllong commented Mar 23, 2021

@JieXiong9119 -- can you resolve the conflicts on this one?

@nllong nllong merged commit 643008f into develop Mar 23, 2021
@nllong nllong deleted the proposals/total-exterior-roof-area branch March 23, 2021 20:13
@JieXiong9119 JieXiong9119 changed the title Proposals/total roof area Add TotalRoofArea Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding new functionality to BuildingSync Non-breaking Change UDFs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants