search

BHoM / Grasshopper_UI

Tools for Grasshopper
GNU Lesser General Public License v3.0
16 stars 5 forks source link

Grasshopper_Toolkit: wires get lost after changing method argument names #561

Closed pawelbaran closed 3 years ago

pawelbaran commented 3 years ago

Description:

Once method argument names get changed, the original component gets deserialized correctly, but the wires connected to the arguments that have been changed disappear.

pawelbaran commented 3 years ago

@adecler it would be great if that could be prioritized in order to avoid getting the scripts broken after https://github.com/BHoM/Revit_Toolkit/pull/887 gets merged. Thanks!

FraserGreenroyd commented 3 years ago

@pawelbaran have you done any versioning for those components?

pawelbaran commented 3 years ago

Yes I tried PreviousVersionAttribute but it did not help. As mentioned by @IsakNaslundBh, versioning will only kick in when deserialization fails - in this case deserialization is fine (argument types did not change), what fails is the wires not being able to find their targets due to changed names. At least that is how I understand it 😉

FraserGreenroyd commented 3 years ago

I'm not sure how any bit of code would be able to identify that a method that originally looked like this

public static bool Foo(int bar, double boo, double hello)

And became

public static bool Foo(int tak, double bok, double goodbye)

Would know to line up immediately 1:1 boo:bok and hello:goodbye without going based on the indexes of the input params. But that could cause other usability problems if the wrong wire gets connected automatically and the user doesn't realise, thus getting a script which appears to be working but isn't?

Maybe I've misunderstood it, but it sounds to me like what you've experienced would be expected behaviour?

pawelbaran commented 3 years ago

How could we then change the method argument names?

adecler commented 3 years ago

I'm not sure how any bit of code would be able to identify that a method that originally looked like this

public static bool Foo(int bar, double boo, double hello)

And became

public static bool Foo(int tak, double bok, double goodbye)

Would know to line up immediately 1:1 boo:bok and hello:goodbye without going based on the indexes of the input params. But that could cause other usability problems if the wrong wire gets connected automatically and the user doesn't realise, thus getting a script which appears to be working but isn't?

Maybe I've misunderstood it, but it sounds to me like what you've experienced would be expected behaviour?

Thanks @FraserGreenroyd , somehow that comment made me feel really understood 😉 . Any new feature we add to the BHoM (like the auto-update of components) quickly becomes a source of complains and bug reporting when it cannot cover 100% of the cases. That caused me quite a few times to refrain from adding new features if I cannot guarantee full coverage even if they would be really useful for the working cases. That being said, versioning is such a key feature that we have no choice but to have it. As you know, the customer is always right so we have no choice but to somehow get it working on all cases as it will always be perceived as a bug otherwise.

@pawelbaran , the code that tries its best to match the old parameter list to the new one is here: https://github.com/BHoM/BHoM_UI/blob/master/UI_Engine/Query/MatchWith.cs . To summarise, this is the underlying logic:

// First, get all the pairs that have matching names
// Then get the ones with matching types if only one candidate each way

So if you change multiple names of arguments with the same types, there will be no safe way to produce a 100% reliable match anymore. If you only change one name per type. We could rely on the order of the parameters, but that will still generate bug issues later when someone change the names and the order and still expect it to magically work.

So how do we solve this ?

I think the best way is to use attributes (again). We could have a OldNames attribute (notice the s) that takes in the new name and the list of old names. That attribute could be applied to a method or to object properties. We could then mine those in the UI (this is not a versioning toolkit issue) to assist the MatchWith method.
What do you think ?

Two additional notes:

pawelbaran commented 3 years ago

Agreed with you @adecler, and, as discussed, happy with any solution or label, as long as it brings us to the happy end 😉 And if impossible to fix, I believe it is an edge case that we could live with (although as you said, that is likely to come back like a boomerang).