-
Notifications
You must be signed in to change notification settings - Fork 184
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
use fromexisting constructor for creating family from name via document #1848
use fromexisting constructor for creating family from name via document #1848
Conversation
|
||
RunCurrentModel(); | ||
|
||
//assert number of loaded families in document: |
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 comment says "assert", but there is no assert here. Not a big deal, but somewhat confusing. The same for the other comment below about asserting.
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.
Can we add a summery of the test somewhere, e.g. sth like This test check to ensure that deleting the family.byName node does not delete the family in Revit host document
var families2 = fec2.Cast<Autodesk.Revit.DB.Family>(); | ||
var newcount = families.Count(); | ||
|
||
Assert.AreEqual(oldcount, newcount); |
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.
The comment about asserting should probably be here for clarity.
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.
Agree
A couple of comments about the comments in the code (minor, change them if you agree, but I'm ok if it stays as it is), but other than that LGTM. |
Purpose
when we create families using the byBame node this implies that the elements already exist in the document and are revit owned - use the from existing constructor to set the
RevitOwned
bool to false.add a test which checks the total number of families before and after removing this node from the graph.
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@ColinDayOrg
FYIs
@QilongTang