-
Notifications
You must be signed in to change notification settings - Fork 2
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
XML_Toolkit: Rename depth property from XMLDepth to Depth #561
Conversation
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.
This was a deliberate decision in #555 because of the issue with having a double Depth
to make this clear it is the XML String representation of Depth. @CKBoulter added a method to query the numerical Depth to avoid confusion.
As seen in this commit and this subsequent commit.
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.
Following discussion with @rboulton-BH - actions to resolve this PR are as follows:
- Change the default return type of the
Query.Depth
method to be0
instead of-1
- Add a description to the
Depth
property of theMarkup
object to explain to the user that this is an XML String representation and they may need to use theQuery.Depth
method to convert to a numerical representation - Add
= Guid.NewGuid();
to the end of the GUID property inXML_oM/Bluebeam/BluebeamObject.cs
- Leave the rename from
XMLDepth
toDepth
- following discussion with @rboulton-BH and explaining the reasoning, he is happy with why the decision was originally made, but is also happy that from the UX perspective for SAP based users (which the Bluebeam pull is for) having the property calledDepth
, with a suitable description, will not cause an issue with their workflows.
Changes requested by @FraserGreenroyd have now been made and checked. |
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.
Just tested the pull and push tests. All passed after following the procedures.
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 3 requests in the queue ahead of you. |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 16 requests in the queue ahead of you. |
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 2 requests in the queue ahead of you. |
This PR is causing a problem with versioning - @rboulton-BH let's pick up early next week to resolve - should be straight forward but I think it needs a second PR on Versioning_Toolkit to accomplish. |
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 1 requests in the queue ahead of you. |
@BHoMBot check installer |
@FraserGreenroyd to confirm, the following actions are now queued:
|
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.
My changes have been addressed, @CKBoulter has approved functionality.
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
|
NOTE: Depends on
BHoM/Versioning_Toolkit#208
Issues addressed by this PR
Closes #560
Rename Bluebeam depth property from XMLDepth to Depth
Test files
Testing should be carried out using XML test procedure: https://burohappold.sharepoint.com/:u:/r/sites/BHoM/02_Current/12_Scripts/03_Alpha/BHoM/XML_Toolkit/XML_Toolkit_PullTest.gh?csf=1&web=1&e=BSk0dH
Changelog
Additional comments