Spatial_Engine: Add HasMergeablePropertiesWith method #1630

kThorsager · 2020-03-31T09:42:41Z

Issues addressed by this PR

Closes #1629
This adds a method which compares the properties of IElements, atm only does base comparison and returns false for objects of equal type which do not have a method specified.

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/EgIee2Xt7DJFu--Isb6FWAkBZot7MPpF8YhyOAKcKy9r8Q?e=sIuvRw

Changelog

Additional comments

Feels like I'm using the Interface method in the exact opposite manner compared to how they're usually used, but I think it's still how we'd want it(?)

does not overlap with #1623

not sure when it would be used, but no reason not to add it

pawelbaran · 2020-03-31T10:00:47Z

Initial comment: if CI/CD is scraping project names from PR titles, then there is a typo in there 🐒

IsakNaslundBh · 2020-04-01T06:49:03Z

Spatial_Engine/Query/IsEqualProperties.cs

+                return false;
+
+            // look for a specific comparing method
+            object result = Reflection.Compute.RunExtensionMethod(element, "IsEqualProperties", new object[] { other });


Wont this lead to stack overflow given that the method is called the same thing?

Hmm, you'd think so, but the test file runs and passes that part, but I'll change it to be on the safe side 👍

pawelbaran

Looks like there is a typo in one of the method names - see code comment. Besides, not sure if Is would be my first choice for prefix here. What about HasEqualProperties?

@al-fisher @adecler

pawelbaran · 2020-04-08T10:00:15Z

Spatial_Engine/Query/IsEqualProperties.cs

+        /****        Private Methods           ****/
+        /******************************************/
+
+        private static bool IIIsEqualProperties(this IElement element, IElement other)


To avoid stack overflow as I have methods calling this method and this method should not call it self or the previous ones

See:
#1630 (comment)

Hmm, makes sense, but looks weird. Maybe it could be done somehow neater (can't come up with anything nice myself though 🐒)?

Think a different name is the way to go without repeting the code or just going with input: (IElement, IElement)

some suggestions if it's the double I which is disturbing

IsEqualPropertiesDispatcher
IsEqualPropertiesProgram
IsEqualPropertiesMeat

IsEqualPropertiesInterface?

kThorsager · 2020-04-08T10:07:08Z

Looks like there is a typo in one of the method names - see code comment. Besides, not sure if Is would be my first choice for prefix here. What about HasEqualProperties?

Yes, I'm open for other names.

An other suggestion is HasMergebleProperties

pawelbaran · 2020-04-08T10:10:16Z

Looks like there is a typo in one of the method names - see code comment. Besides, not sure if Is would be my first choice for prefix here. What about HasEqualProperties?

I'm open for other names, other suggestion is HasMergebleProperties

Even better! Possibly we could have 2 methods, one checking whether the properties are equal and the other checking if they can be merged. Something for Diffing_Engine, @alelom?

Dropping one more idea: CanMergeProperties

Dismissing my change request and turning into comments.

al-fisher · 2020-04-08T14:25:59Z

I like HaveMergebleProperties as a naming convention. "Have" though rather than "Has" as is operating on a pair of objects.

Spatial_Engine/Query/IsEqualProperties.cs

IsakNaslundBh

Think the code generally looks fine now. File needs to be renamed HaveMergebleProperties though.

FraserGreenroyd · 2020-04-08T16:50:36Z

/azp run BHoM_Engine.CheckCompliance

azure-pipelines · 2020-04-08T16:50:43Z

Azure Pipelines successfully started running 1 pipeline(s).

adecler

The file name is now incorrect.
Shouldn't it be mergeable instead of mergeble?
I am not convinced by the use of have here. The method is applied to the this argument only not to all the inputs together (i.e. x.HasMergeableProperties(y), not (x&y).HaveMergeableProperties()). I would be fine with x.HasMergeablePropertiesWith(y) though.

kThorsager · 2020-04-09T07:14:55Z

The file name is now incorrect.

Yes, fixed

Shouldn't it be mergeable instead of mergeble?

Yes, fixed

I am not convinced by the use of have here. The method is applied to the this argument only not to all the inputs together (i.e. x.HasMergeableProperties(y), not (x&y).HaveMergeableProperties()). I would be fine with x.HasMergeablePropertiesWith(y) though.

Maybe, will hold off on action as it sounds like there should be some discussion and am unsure myself

al-fisher · 2020-04-10T15:10:05Z

I am not convinced by the use of have here. The method is applied to the this argument only not to all the inputs together (i.e. x.HasMergeableProperties(y), not (x&y).HaveMergeableProperties()). I would be fine with x.HasMergeablePropertiesWith(y) though.

Yes - I like x.HasMergeablePropertiesWith(y) more @adecler
Great point. It was indeed the this keyword that was throwing me to start with. “Have” did not really scan perfectly. Appending “With” is neat and could be a pattern to reuse elsewhere for clarity.

adecler

LGTM on the side of the code.
Probably good to have another approval from someone involved in teh Spatial toolkit itself

adecler · 2020-04-15T08:41:49Z

/azp run BHoM_Engine.CheckInstaller

azure-pipelines · 2020-04-15T08:41:58Z

Azure Pipelines successfully started running 1 pipeline(s).

IsakNaslundBh

Now all looks fine!

kThorsager added 2 commits March 31, 2020 10:35

IsEqualProperties

e94741b

IElement0D

78eec47

not sure when it would be used, but no reason not to add it

kThorsager added the type:feature New capability or enhancement label Mar 31, 2020

kThorsager added this to the BHoM 3.2 β MVP milestone Mar 31, 2020

kThorsager requested review from rwemay, IsakNaslundBh and pawelbaran March 31, 2020 09:42

kThorsager requested a review from al-fisher as a code owner March 31, 2020 09:42

kThorsager self-assigned this Mar 31, 2020

kThorsager mentioned this pull request Mar 31, 2020

Spatial_Engine: Add default behaviour for IElement2D extension methods #1623

Merged

kThorsager changed the title ~~Spatail_Engine: Add IsEqualProperties method~~ Spatial_Engine: Add IsEqualProperties method Mar 31, 2020

IsakNaslundBh reviewed Apr 1, 2020

View reviewed changes

InterfaceIntefaceMethod

c2899f7

kThorsager requested a review from IsakNaslundBh April 1, 2020 09:47

Merge branch 'master' into Spatial_Engine-#1629-IsEqualProperties

64b5afd

pawelbaran previously requested changes Apr 8, 2020

View reviewed changes

IsakNaslundBh reviewed Apr 8, 2020

View reviewed changes

Spatial_Engine/Query/IsEqualProperties.cs Outdated Show resolved Hide resolved

IsakNaslundBh reviewed Apr 8, 2020

View reviewed changes

Spatial_Engine/Query/IsEqualProperties.cs Outdated Show resolved Hide resolved

Renaming and default to false

51947ff

kThorsager changed the title ~~Spatial_Engine: Add IsEqualProperties method~~ Spatial_Engine: Add HaveMergebleProperties method Apr 8, 2020

kThorsager requested a review from IsakNaslundBh April 8, 2020 14:44

FraserGreenroyd requested a review from pawelbaran April 8, 2020 15:55

IsakNaslundBh requested changes Apr 8, 2020

View reviewed changes

adecler requested changes Apr 9, 2020

View reviewed changes

Fix filename and spelling

6a78d11

Name change to HasMergeablePropertiesWith

3e27993

kThorsager requested review from adecler and IsakNaslundBh April 14, 2020 07:42

kThorsager changed the title ~~Spatial_Engine: Add HaveMergebleProperties method~~ Spatial_Engine: Add HasMergeablePropertiesWith method Apr 14, 2020

adecler approved these changes Apr 15, 2020

View reviewed changes

IsakNaslundBh approved these changes Apr 15, 2020

View reviewed changes

IsakNaslundBh merged commit 9b2e0a0 into master Apr 15, 2020

IsakNaslundBh deleted the Spatial_Engine-#1629-IsEqualProperties branch April 15, 2020 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spatial_Engine: Add HasMergeablePropertiesWith method #1630

Spatial_Engine: Add HasMergeablePropertiesWith method #1630

kThorsager commented Mar 31, 2020 •

edited

Loading

pawelbaran commented Mar 31, 2020

IsakNaslundBh Apr 1, 2020

kThorsager Apr 1, 2020

pawelbaran left a comment

pawelbaran Apr 8, 2020

kThorsager Apr 8, 2020

pawelbaran Apr 8, 2020

kThorsager Apr 8, 2020

pawelbaran Apr 8, 2020

kThorsager commented Apr 8, 2020 •

edited

Loading

pawelbaran commented Apr 8, 2020

al-fisher commented Apr 8, 2020

IsakNaslundBh left a comment

FraserGreenroyd commented Apr 8, 2020

azure-pipelines bot commented Apr 8, 2020

adecler left a comment

kThorsager commented Apr 9, 2020

al-fisher commented Apr 10, 2020

adecler left a comment

adecler commented Apr 15, 2020

azure-pipelines bot commented Apr 15, 2020

IsakNaslundBh left a comment

Spatial_Engine: Add HasMergeablePropertiesWith method #1630

Spatial_Engine: Add HasMergeablePropertiesWith method #1630

Conversation

kThorsager commented Mar 31, 2020 • edited Loading

Issues addressed by this PR

Test files

Changelog

Additional comments

pawelbaran commented Mar 31, 2020

IsakNaslundBh Apr 1, 2020

Choose a reason for hiding this comment

kThorsager Apr 1, 2020

Choose a reason for hiding this comment

pawelbaran left a comment

Choose a reason for hiding this comment

pawelbaran Apr 8, 2020

Choose a reason for hiding this comment

kThorsager Apr 8, 2020

Choose a reason for hiding this comment

pawelbaran Apr 8, 2020

Choose a reason for hiding this comment

kThorsager Apr 8, 2020

Choose a reason for hiding this comment

pawelbaran Apr 8, 2020

Choose a reason for hiding this comment

kThorsager commented Apr 8, 2020 • edited Loading

pawelbaran commented Apr 8, 2020

al-fisher commented Apr 8, 2020

IsakNaslundBh left a comment

Choose a reason for hiding this comment

FraserGreenroyd commented Apr 8, 2020

azure-pipelines bot commented Apr 8, 2020

adecler left a comment

Choose a reason for hiding this comment

kThorsager commented Apr 9, 2020

al-fisher commented Apr 10, 2020

adecler left a comment

Choose a reason for hiding this comment

adecler commented Apr 15, 2020

azure-pipelines bot commented Apr 15, 2020

IsakNaslundBh left a comment

Choose a reason for hiding this comment

kThorsager commented Mar 31, 2020 •

edited

Loading

kThorsager commented Apr 8, 2020 •

edited

Loading