Fixes #4385 about calling the Create methods when loading models from disk #4485
Conversation
…have the same visibility.
| // Choose the one that is public, if both are non-public choose the one that isn't private | ||
| // if they have the same visibility, then throw an exception, since this shouldn't happen. | ||
| if(ctor != null && create != null) | ||
| { |
There was a problem hiding this comment.
I don't really like this nested if's, I am not sure if they're legible enough, and it is ambiguous what should happen if there's a 'protected' create or constructor method (which, I believe, never happens in the codebase...). Still, this gets the job done.
I can think of a couple of ways of making this, but not sure if they would be more legible. Please, let me know if I should rewrite this in another way. #Resolved
There was a problem hiding this comment.
So I've changed this nested if's in a new iteration (see this comment) but I am not sure if I prefer the nested of's or the new solution. #Resolved
| { | ||
| if(ctor.IsPublic || ctor.IsPrivate == create.IsPrivate) | ||
| { | ||
| throw Contracts.Except($"Can't load type {instType}, because it has both create and constructor methods with the same visibility. Please open an issue for this to be fixed."); |
There was a problem hiding this comment.
I didn't know what text to put in the Exception. Please let me know if I should change it. #Resolved
There was a problem hiding this comment.
I would change "please open an issue..." to something like: "Please indicate which one should be used by changing either the signature or the visibility of one of them".
In reply to: 347643643 [](ancestors = 347643643)
Codecov Report
@@ Coverage Diff @@
## master #4485 +/- ##
==========================================
+ Coverage 74.9% 75.07% +0.16%
==========================================
Files 908 908
Lines 160072 160126 +54
Branches 17222 17240 +18
==========================================
+ Hits 119903 120210 +307
+ Misses 35359 35094 -265
- Partials 4810 4822 +12
|
| // If they have the same visibility, then throw an exception, since this shouldn't happen. | ||
|
|
||
| if (ctor.Accessmodifier() == create.Accessmodifier()) | ||
| { |
There was a problem hiding this comment.
So I changed the nested if's I had (link to comment) for this other solution using the Accessmodifier() extension method (as suggested by @yaeldekel ). Although I think this one is more legible, I wouldn't be sure if it's worth it to create the extension method only for this.... So let me know your opinions, Thanks! #Resolved
There was a problem hiding this comment.
To me this seems cleaner.
A couple of more ways you can decrease the amount of "if/else"s in the code:
- you can put the assignment inside the if condition, like this:
if ((ctor = loaderType.GetConstructor(...)) == null)
- If you throw or return inside the "if", then you don't need the "else":
if (ctor.Accessmodifier() == create.Accessmodifier())
throw ...
if (ctor.Accessmodifier() > create.Accessmodifier())
{
...
return true
}
if (ctor.Accessmodifier() < create.Accessmodifier())
...
etc.
In reply to: 347688260 [](ancestors = 347688260)
ghost
locked as resolved and limited conversation to collaborators
Fixes #4385
As concluded in the discussion there, if a class has both a constructor and a create method that matches the parameter types that the
ComponentCatalogis looking for, then it should use the one which is public. If both are non-public, then it should use the internal one. If both have the same visibility then it should throw an exception.For this to happen, I modified the
ComponentCatalogto work as described. I also had to change the visibility of several methods and constructors to work as described. Since, as per @yaeldekel 's instructions, the create method should be called instead of the constructor in all cases, then I did the following: