-
Notifications
You must be signed in to change notification settings - Fork 437
Resolve #750 - Remove Explicit Interface Implementations #751
Conversation
Codecov Report
@@ Coverage Diff @@
## indev #751 +/- ##
=======================================
+ Coverage 1.88% 1.9% +0.02%
=======================================
Files 131 131
Lines 7816 7627 -189
Branches 616 581 -35
=======================================
- Hits 147 145 -2
+ Misses 7669 7482 -187
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some problematic hierarchy in the interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with some questions, seems like most of hierarchy problems get solved, there is one more refactor PR that is going to be full of conflicts from this with some important change, I hope it get merged, I think this and #745 are the last huge refactoring for the moment.
GetDistanceTo(u) > DETECT_RANGE || // in range | ||
!_game.ObjectManager.TeamHasVisionOn(Team, u)) // visible to this minion | ||
continue; // If not, look for something else | ||
if (!(it.Value is IAttackableUnit u) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you removed the comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cause to my eyes, the comments literally repeat what the code describes itself already pretty much
public static Item CreateFromType(Inventory inventory, ItemType item) | ||
public void SetStacks(byte newStacks) | ||
{ | ||
throw new System.NotImplementedException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open issue about that, need to be fixed fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do that, currently this function is only used by buffs which is why this doesnt matter too much in itself
public int RecipeItem2 { get; private set; } | ||
public int RecipeItem3 { get; private set; } | ||
public int RecipeItem4 { get; private set; } | ||
private int RecipeItem1 { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is fixed in another PR so you don't need to do array here cause it already fixed there (we should open issue about that and comment that it already fixed in another unmerged PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea i changed it a bit only cause of the talk that resulted afterwards. pretty sure there is no open PR about this atm though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#745 Changed that to array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i see, thats kind of offtopic for that pr i suppose. well ill keep it like this now though given that the zip PR isnt in mergeable state yet either so he can just overwite it with his
Closes #750
Refs LeagueSandbox/LeagueSandbox-Default#85