Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Message.Attach picks wrong method because of similar name #314

Open
taori opened this issue Apr 10, 2016 · 9 comments · May be fixed by #739
Open

Message.Attach picks wrong method because of similar name #314

taori opened this issue Apr 10, 2016 · 9 comments · May be fixed by #739

Comments

@taori
Copy link
Contributor

taori commented Apr 10, 2016

Hello.

I've experienced an issue with this commit: taori/SE-Calculator@9a168ba

The error i'm running into is with NewShipViewModel.cs basically i have 2 methods with the same name where only the parameter type is different. However it will only enter the method with Delete(Thruster item) if i click on both types. For an energy source it too will enter the method with a thruster, but pass a null as parameter.

I suppose a little fix is required for caliburn to pick the correct method here?

@nigel-sampson nigel-sampson added this to the v3.1.0 milestone Apr 14, 2016
@nigel-sampson
Copy link
Contributor

Yup, at the moment it's only looking at count of the parameters and not their types.

It should be possible to do type examination.

@StephenWard
Copy link

@nigel this would be a significant change from Caliburn's original explicit goal of keeping the Message.Attach functionality extremely limited.

  • would the new implementation handle derived types?
  • interface parameters that are given concrete objects e.g MyMethod(ICollection list) ?
  • generics?
  • generics w/ variance ("in" and "out")?

@taori
Copy link
Contributor Author

taori commented Jun 28, 2016

The functionality would still be limited. Making it pick the correct method in an overload scenario would be a minor extension to prevent unexpected bugs.

@nigel-sampson nigel-sampson modified the milestones: v4.0.0, v3.1.0 Jun 28, 2016
@nigel-sampson
Copy link
Contributor

@StephenWard I don't think it's a major expansion, we won't be adding more capabilities to Message.Attach, just a better resolver for which method to call when there are overloads.

Thinking over this some more I believe this could be a breaking change if people are depending on the current behavior.

In terms of the scenarios listed above staying away from generics is probably best but the others can be resolved by checking IsAssignableFrom.

@Athari
Copy link

Athari commented Jul 27, 2016

Why not use C# binder from Microsoft.CSharp.dll? It'll choose an override just like C# does. Not sure about its availability on all platforms though.

@nigel-sampson
Copy link
Contributor

Can you expand on that, it's nothing something I've used before. But platform availability would certainly be a concern.

@Athari
Copy link

Athari commented Jul 27, 2016

DLR is used by C# compiler to implement dynamic keyword. Binder isn't well-documented, but it's mostly about creating weird CallSites like CallSite<Func<CallSite, Type, T1, T2, TObject>> and passing results from Binder's methods to them.

See Dynamitey for sample code, it includes all possible operations. It adds an extra caching level and includes workarounds for bugs (with static methods, iirc), so the code may look more complex than actually needed. Here's a very simplified code I use in a similar scenario for user-provided converters. JSON.NET uses it too, though only for getters and setters and with an additional layer of reflection over it. Considering availability of JSON.NET on many platforms, I think you can rely on its #ifs to see where it's available.

@nigel-sampson
Copy link
Contributor

Thanks for that, will look it over, may be a bit of sledgehammer to crack a nut, but a definite possibility.

@Athari
Copy link

Athari commented Jul 27, 2016

Considering DLR is built-in and exactly matches behavior of C#, I don't think it qualifies as a sledgehammer. I'd say using anything else would be reinventing the wheel.

@nigel-sampson nigel-sampson added this to Backlog in v4.0.0 Development Apr 24, 2019
@KasperSK KasperSK linked a pull request Jan 29, 2021 that will close this issue
@KasperSK KasperSK moved this from Backlog to In Progress in v4.0.0 Development Jul 29, 2021
@vb2ae vb2ae modified the milestones: v4.0.0, Version 5.0 Nov 22, 2021
@vb2ae vb2ae added this to To do in Caliburn.Micro V5 Dec 23, 2021
@vb2ae vb2ae moved this from To do to In progress in Caliburn.Micro V5 Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v4.0.0 Development
  
In Progress
Development

Successfully merging a pull request may close this issue.

5 participants