All Modify methods should return void

[adecler]

Following conversation on https://github.com/BHoM/admin/issues/9 and the fact that object are always immutable in UI and always mutable in C#, it would makes sense for the modify methods to return void.

Once the issue in https://github.com/BHoM/admin/issues/9 is resolve, I can easily change the UI to act as if the Modify methods are returning void and we can then start refactoring the modify methods in the code.

The idea would be to change the UI side after sprint 4 but action this issue next milestone.

[FraserGreenroyd]

@adecler just getting a head start on some planning - how likely are we to be closing this one out this milestone and what sort of actions do we need to undertake? I'm imagining there'll be a few coordination bits on something of this level?

[adecler]

@FraserGreenroyd , yes, we should try to get this sorted once and for all.

As for the actions, @al-fisher summarised them here: https://github.com/BHoM/BHoM/discussions/1031#discussioncomment-106258

I'll copy the most relevant bits here for simplicity:

To summarise:

• Requirement for new sub categories of the modify class based on the return type of the method.
  • return void: the object is modified directly
  • returning a type: the object returned is a new one
• Compute methods are not extension methods

If happy with above (please do confirm/add any comments for further tweaks) - suggest we then plan to:

1. Update Compliance guidelines and Wiki etc.
2. Ensure the Framework supports both the new rules, and also versioning of legacy code as mentioned on call. Any issues here to be raised, if not already, and closed in 4.0
3. Then finally, also start raising some issues across engines to update methods as needed - planned for close out in 4.1. After completion of the above

[FraserGreenroyd]

Ok sounds like some changes needed on Test Toolkit around the compliance checks for modify methods, plus an update to the wiki pages to reflect that from the compliance side which should be easy enough to handle.

returning a type: the object returned is a new one

From the linked discussion:

Manipulate an object returning new object/type of object(s)

This suggests that modify methods which do not return void can return an object of any type (not necessarily matching an input parameter), is that understanding correct?

If so, do we have any indication of when a method should return void vs a new object that the compliance check can reliably use? If not then I guess the compliance check needs to come out and go to human judgement?

Ensure the Framework supports both the new rules, and also versioning of legacy code as mentioned on call. Any issues here to be raised, if not already, and closed in 4.0

I probably missed it in the flurry of activity last milestone, has this been done? Or is it still waiting for #9 to be fully closed out?

I feel it may be worth a half hour coordination session with the relevant people would be good (to avoid the back/forth in the comments cause I think I have a few questions :smile: ) - looks like we could squeeze in a half hour before the framework planning session next Tuesday which @IsakNaslundBh ought to be back for? :smile:

[adecler]

I feel it may be worth a half hour coordination session with the relevant people would be good (to avoid the back/forth in the comments cause I think I have a few questions 😄 ) - looks like we could squeeze in a half hour before the framework planning session next Tuesday which @IsakNaslundBh ought to be back for? 😄

Sounds good to me 👍

[FraserGreenroyd]

Based on the call today with @adecler @IsakNaslundBh @al-fisher @rolyhudson @alelom @pawelbaran @LMarkowski

This is based on order of discussion, not priority.

1) Resolve BHoM_UI to handle void return methods @adecler 2) Resolve unit tests to aid checking that there are no issues with versioning/serialisation @adecler 3) Checking RunExtensionMethod can handle void return @IsakNaslundBh 4) Compliance rule is all modify methods should return void UNLESS they are an Immutable OR returning a totally different type @FraserGreenroyd 5) Update the wiki to make this clear @al-fisher

Priority order:

• @adecler to update the framework by end of sprint 2 latest
• @IsakNaslundBh to confirm RunExtensionMethod will work by end of sprint 2 latest (work with @adecler if changes needed)
• Everyone else to then develop unit tests and switch over to void return type in sprints 2, 3, and 4.

[IsakNaslundBh]

Checking RunExtensionMethod can handle void return @IsakNaslundBh

Have done a first check of this now, and as far as I can see, run extension method is behaving just fine. What I tested was adding the following test methods:

        public static void TranslateTest(this Point pt, Vector transform)
        {
            pt.X += transform.X;
            pt.Y += transform.Y;
            pt.Z += transform.Z;
        }

        /***************************************************/

        public static void TranslateTestRunExt(this Point pt, Vector transform)
        {
            BH.Engine.Reflection.Compute.RunExtensionMethod(pt, "TranslateTest", new object[] { transform });
        }

Running in Grasshopper gives the following:

Based on this, I assume things will run fine without any need to change. Ofc good to always keep an eye on it when we get to the real cases, but seem fine.