BHoM_Engine: Add and Query Fragments #1185

michaldengusiak · 2019-09-11T07:39:17Z

NOTE: Depends on

Issues addressed by this PR

Closes #1184

Add Modify AddFragments

Test files

N/A

Changelog

Add node to Modify AddFragments
Add node Query FindFragment

Additional comments

FraserGreenroyd

This is not the right place for a method which will be needed by so many disciplines. This will result in duplicated code in every engine.

michaldengusiak · 2019-09-11T11:30:08Z

ok, I implemented comments and moved to the correct place as advice

FraserGreenroyd

PR

Change PR title to reflect the actual engine being modified and to correctly identify the change
Bullet point the change log as there are 2 changes here

Code

See comments on the code

FraserGreenroyd · 2019-09-11T13:52:39Z

BHoM_Engine/Modify/AddFragment.cs

+        [Input("iBHoMObject", "Any object implementing the IBHoMObject interface that can have fragment properties appended to it")]
+        [Input("fragment", "Any fragment object implementing the IBHoMFragment interface to append to the object")]
+        [Output("iBHoMObject", "The BHoM object with the added fragment")]
+        public static IBHoMObject AddFragment(this IBHoMObject iBHoMObject, IBHoMFragment fragment)


As per the discussion on #1184 I would like to see an optional bool (set to false initially) that allows users to replace fragments if they already exist.
Similarly, if the input is false, we should check if the fragment exists and return an error if it does. See discussion on #1184 for more info.

Another thing on this method, You will need to clone the object to be changed as well as its list before modifying it. Either use the DeepClone method in the BHoMEngine, or shallow clone the object, and copy the fragment list. Right now, the incoming objects will be changed, changing things upstream in an visual programming workflow.

IsakNaslundBh

added comment

IsakNaslundBh · 2019-09-12T06:59:29Z

BHoM_Engine/Modify/AddFragment.cs

+        [Input("iBHoMObject", "Any object implementing the IBHoMObject interface that can have fragment properties appended to it")]
+        [Input("fragment", "Any fragment object implementing the IBHoMFragment interface to append to the object")]
+        [Output("iBHoMObject", "The BHoM object with the added fragment")]
+        public static IBHoMObject AddFragment(this IBHoMObject iBHoMObject, IBHoMFragment fragment)


Another thing on this method, You will need to clone the object to be changed as well as its list before modifying it. Either use the DeepClone method in the BHoMEngine, or shallow clone the object, and copy the fragment list. Right now, the incoming objects will be changed, changing things upstream in an visual programming workflow.

FraserGreenroyd

My previous comments have not yet been addressed. Requesting changes to ensure they are addressed before this is merged.

FraserGreenroyd

This is a good improvement, some minor comments relating to style and documentation in the code (functionality is good). See below.

PR

Change PR title to reflect the actual engine being modified and to correctly identify the change
Bullet point the change log as there are 2 changes here

Code

See comments on the code

FraserGreenroyd · 2019-09-13T18:47:35Z

BHoM_Engine/Modify/AddFragment.cs

+            if(o.Fragments.Contains(fragment))
+            {
+                if(replace)
+                {


Use single line if statements given these are single line operations

FraserGreenroyd · 2019-09-13T18:49:09Z

BHoM_Engine/Modify/AddFragment.cs

+                }
+                else
+                {
+                    Reflection.Compute.RecordError("Fragment already exists");


Can we expand this error message to tell the user something like "That fragment already exists on this object. If you would like to replace the existing fragment set the 'replace' input to 'true'"

FraserGreenroyd · 2019-09-13T18:50:26Z

BHoM_Engine/Modify/AddFragment.cs

+        [Description("Appends a Fragment Property to a given BHoM Object")]
+        [Input("iBHoMObject", "Any object implementing the IBHoMObject interface that can have fragment properties appended to it")]
+        [Input("fragment", "Any fragment object implementing the IBHoMFragment interface to append to the object")]
+        [Input("replace", "If true will replace exisiting fragments ")]


Expand the description to be more informative for the user, Something like "If set to true and the object already contains a fragment of the type being added, the fragment will be replaced by this instance"

FraserGreenroyd

Finally LGTM

FraserGreenroyd · 2019-09-14T20:54:19Z

While this LGTM now and will be merged in to unblock on the workflows, @michaldengusiak please can you raise issues to deprecate the existing methods within the Environment Engine which do similar functionality. Please assign to me and put on the 2.4RC milestone. Cheers!

AddFragments

ed431a5

michaldengusiak requested review from FraserGreenroyd and IsakNaslundBh September 11, 2019 07:39

michaldengusiak requested review from adecler and al-fisher as code owners September 11, 2019 07:39

michaldengusiak self-assigned this Sep 11, 2019

FraserGreenroyd requested changes Sep 11, 2019

View reviewed changes

added adjustment

00fa783

michaldengusiak requested a review from FraserGreenroyd September 11, 2019 11:30

FraserGreenroyd requested changes Sep 11, 2019

View reviewed changes

IsakNaslundBh reviewed Sep 12, 2019

View reviewed changes

Isak comments

097af49

michaldengusiak requested review from FraserGreenroyd and IsakNaslundBh September 12, 2019 11:31

FraserGreenroyd requested changes Sep 12, 2019

View reviewed changes

update per comments

5529373

michaldengusiak requested a review from FraserGreenroyd September 13, 2019 14:50

FraserGreenroyd requested changes Sep 13, 2019

View reviewed changes

michaldengusiak changed the title ~~Physical_Engine: Add Modify AddFragments~~ BHoM_Engine: Add Modify AddFragments Sep 14, 2019

implement comments

6e3e2b9

michaldengusiak requested a review from FraserGreenroyd September 14, 2019 20:30

FraserGreenroyd changed the title ~~BHoM_Engine: Add Modify AddFragments~~ BHoM_Engine: Add and Query Fragments Sep 14, 2019

FraserGreenroyd approved these changes Sep 14, 2019

View reviewed changes

FraserGreenroyd merged commit d5cc6a6 into master Sep 14, 2019

FraserGreenroyd deleted the Physical_Engine-Issue1184-AddFragments branch September 14, 2019 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BHoM_Engine: Add and Query Fragments #1185

BHoM_Engine: Add and Query Fragments #1185

michaldengusiak commented Sep 11, 2019 •

edited

Loading

FraserGreenroyd left a comment

michaldengusiak commented Sep 11, 2019

FraserGreenroyd left a comment

FraserGreenroyd Sep 11, 2019

IsakNaslundBh Sep 12, 2019

IsakNaslundBh left a comment

IsakNaslundBh Sep 12, 2019

FraserGreenroyd left a comment

FraserGreenroyd left a comment

FraserGreenroyd Sep 13, 2019

michaldengusiak Sep 14, 2019

FraserGreenroyd Sep 13, 2019

michaldengusiak Sep 14, 2019

FraserGreenroyd Sep 13, 2019

michaldengusiak Sep 14, 2019

FraserGreenroyd left a comment

FraserGreenroyd commented Sep 14, 2019

BHoM_Engine: Add and Query Fragments #1185

BHoM_Engine: Add and Query Fragments #1185

Conversation

michaldengusiak commented Sep 11, 2019 • edited Loading

NOTE: Depends on

Issues addressed by this PR

Test files

Changelog

Additional comments

FraserGreenroyd left a comment

Choose a reason for hiding this comment

michaldengusiak commented Sep 11, 2019

FraserGreenroyd left a comment

Choose a reason for hiding this comment

PR

Code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IsakNaslundBh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FraserGreenroyd left a comment

Choose a reason for hiding this comment

FraserGreenroyd left a comment

Choose a reason for hiding this comment

PR

Code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FraserGreenroyd left a comment

Choose a reason for hiding this comment

FraserGreenroyd commented Sep 14, 2019

michaldengusiak commented Sep 11, 2019 •

edited

Loading