Element type nodes #2468
Element type nodes #2468
Conversation
* Element.GetSubComponents (#16) * Element.GetSubComponents node with icons * Element.GetSubComponents tests * ElementTypeByName node * ElementTypeByName tests * Created ElemetType class * made types inherit from ElementType * node images + small fixes * element and wall type tests * Update WallTypeTests.cs * ElementType tests * WallType, FloorType and RoofType tests * Revert sample DYN files This reverts commit 38bd219. * comment updates * change var to int
SHKnudsen
requested review from
mjkkirschner,
AndyDu1985,
radumg and
ZiyunShang
|
This PR has API breaking implications. You must be very careful when refactoring like this - Code which previously was written on top of the DynamoRevit node classes now has the potential to break. It's difficult to say what will happen unless it is throughly tested. But I know for certain that this breaks a few public APIs and as such should not be released as a DynamoRevit 2.x change. In general you should not change the type or signature of public members until you are ready to break an API. https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net for example, this similar PR broke lots of code: If you intend to get this into 2.x then I would act as conservatively as possible, do not refactor these classes, but add additional methods to If you intend to get this into 3.x then leave this PR open and maintain it (merging master) until DynamoRevit intentionally decides to break API compatibility- that could be today! - but thats for @ZiyunShang @AndyDu1985 and Minjie to decide think. |
|
Thanks for the notes @mjkkirschner , it's an interesting issue! I hadn't originally flagged this as an issue in our internal review as the signature of the classes will essentially resolve fine at run-time, but now realise I hadn't considered whether Dynamo uses fully qualified names when compiling nodes/etc. Would be great if you could give us a few examples of the APIs that you know will break, would love to test those a bit more. If it's still an issue, we can take a more conservative approach, no probs. |
| /// </summary> | ||
| /// <returns name="ElementType">Element Type or Null.</returns> | ||
| public Element ElementType | ||
| public ElementType ElementType |
There was a problem hiding this comment.
@radumg here is an example of a simple API break - this class no longer has as property called Element.
| using RevitServices.Persistence; | ||
|
|
||
| namespace Revit.Elements | ||
| { | ||
| /// <summary> | ||
| /// A Revit RoofType | ||
| /// </summary> | ||
| public class RoofType : Element | ||
| public class RoofType : ElementType |
There was a problem hiding this comment.
for any of these types which now have new base classes, it might be fine, or it might not, it's worth considering a package which used this type as the input to a node or even just internally, will this code need to be recompiled? Will it need to have changes made and then recompiled?
Can you prove it won't?
There was a problem hiding this comment.
also, it looks like you did not have to modify any existing tests? That is usually a good sign for these type of refactors.
I think the best that can be done, if you want to move forward with a refactor like this is to try this change out on a package that was compiled against an old version of DynamoRevit and see what happens. This can be run as an automated test.
also, please see the one change which looks to be a genuine break.
|
@SHKnudsen This PR cause build failure, I've revert it, could you resend a new PR? Thanks! |
Purpose
This PR introduces an ElementType category to the Dynamo library, all other Dynamo types (WallType, FloorType, etc.) now inherits from ElementType which means a lot of duplicated code has been removed.
The following new nodes have been added:
ElementType nodes
WallType nodes
FloorType nodes
RoofType nodes
All of the nodes have related unit and system tests, which has been run in RTF before submitting.
Unit Test
System Test
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@AndyDu1985
@radumg
@mjkkirschner
@ZiyunShang
FYIs
Screenshots of the dynamo graphs for the system test
ElementType Properties test
ElementType by name test
Duplicate ElementType test
ElementType get preview image test
WallType Properties test
WallType Thermal Properties test
Check if FloorType is foundation slab test
get FloorType structural material test
FloorType thermal properties test
RoofType thermal properties test