Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

Resolve #750 - Remove Explicit Interface Implementations #751

Merged
merged 12 commits into from
Nov 22, 2018
Merged

Resolve #750 - Remove Explicit Interface Implementations #751

merged 12 commits into from
Nov 22, 2018

Conversation

Veykril
Copy link
Contributor

@Veykril Veykril commented Oct 3, 2018

@codecov-io
Copy link

codecov-io commented Oct 3, 2018

Codecov Report

Merging #751 into indev will increase coverage by 0.02%.
The diff coverage is 1.63%.

Impacted file tree graph

@@           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
Impacted Files Coverage Δ
...Buildings/AnimatedBuildings/ObjAnimatedBuilding.cs 0% <ø> (ø) ⬆️
GameServerLib/Packets/PacketHandlers/HandleMove.cs 0% <ø> (ø) ⬆️
GameServerLib/API/ApiFunctionManager.cs 4.57% <ø> (ø) ⬆️
GameServerLib/Content/NavGrid.cs 0% <ø> (ø) ⬆️
GameServerLib/GameObjects/Other/Target.cs 0% <ø> (ø) ⬆️
GameServerLib/Scripting/CSharp/GameScriptEmpty.cs 0% <ø> (ø) ⬆️
...meObjects/AttackableUnits/Buildings/ObjBuilding.cs 0% <ø> (ø) ⬆️
GameServerLib/Chatbox/Commands/KillCommand.cs 0% <0%> (ø) ⬆️
GameServerLib/Items/ItemData.cs 0% <0%> (ø)
GameServerLib/GameObjects/Missiles/Cone.cs 0% <0%> (ø) ⬆️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3e2803...7656c14. Read the comment docs.

Copy link
Contributor

@danil179 danil179 left a 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

GameServerCore/Domain/GameObjects/IChampion.cs Outdated Show resolved Hide resolved
GameServerCore/Domain/GameObjects/IGameObject.cs Outdated Show resolved Hide resolved
GameServerCore/Domain/GameObjects/IGameObject.cs Outdated Show resolved Hide resolved
GameServerCore/Domain/GameObjects/IGameObject.cs Outdated Show resolved Hide resolved
GameServerCore/Domain/IInventoryManager.cs Outdated Show resolved Hide resolved
GameServerCore/Domain/IItem.cs Outdated Show resolved Hide resolved
@Veykril Veykril changed the title Remove Explicit Interface Implementations [WIP] Remove Explicit Interface Implementations Oct 4, 2018
@Veykril Veykril changed the title [WIP] Remove Explicit Interface Implementations Remove Explicit Interface Implementations Oct 5, 2018
@Veykril Veykril changed the title Remove Explicit Interface Implementations Resolve #750 - Remove Explicit Interface Implementations Oct 6, 2018
Copy link
Contributor

@danil179 danil179 left a 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) ||
Copy link
Contributor

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?

Copy link
Contributor Author

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

GameServerLib/GameObjects/Missiles/Cone.cs Outdated Show resolved Hide resolved
public static Item CreateFromType(Inventory inventory, ItemType item)
public void SetStacks(byte newStacks)
{
throw new System.NotImplementedException();
Copy link
Contributor

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.

Copy link
Contributor Author

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; }
Copy link
Contributor

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).

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Veykril added a commit to LeagueSandbox/LeagueSandbox-Default that referenced this pull request Nov 22, 2018
@Veykril Veykril merged commit f1c7bdc into LeagueSandbox:indev Nov 22, 2018
@Veykril Veykril deleted the interface_fixes branch November 22, 2018 20:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants