Try to split player service (To review)

[raskyer]

Short Description:
Try to make services inside this mess (first incrementation)

[douglascamata]

Merge conflicts :/

[raskyer]

@douglascamata fix

[raskyer]

Got other stuff to fix

[elicwhite]

can't this whole thing be

r = response.get('responses', {}).get('GET_INVENTORY', {}).get('inventory_delta', {}).get('inventory_items', {})

[fredrik-hellmangroup]

what was the 'fix' commit a4c7426 ?

[douglascamata]

@LeaklessGfy you mixed a lot of fixes with code move and we're a bit afraid to merge this. Can you organise commits in a better way? Maybe split this PR in 2 (or more):

1. Just code extraction from bot's main class
2. Fixes to the extracted code

[jtdroste]

Please use the .get method on dicts, rather than checking every nested level and returning if it doesn't exist. This extends @TheSavior's comment - however there's two other areas that are just like it that I'd like you to change.

In the future PLEASE title your commits appropriately. Having a commit message say "Fix" gives us NO information what was changed

[bstpierr]

Please remove all commented-out code.

[raskyer]

All the comment are pretty interesting because I just move code on this PR. No new function so these comment concern former dev' not me especially. But I can do that of curse.

This merge is important because it's the first step to get thing more modular and structured.

[elicwhite]

@LeaklessGfy I think @douglascamata's last comment represents accurately where the dev team feels about this:

@LeaklessGfy you mixed a lot of fixes with code move and we're a bit afraid to merge this. Can you organise commits in a better way? Maybe split this PR in 2 (or more):

Just code extraction from bot's main class
Fixes to the extracted code

We are scared of this large of a refactor. It's too hard to feel confident in it from looking at the diff. If there is any way to split it up into smaller PRs, it would be beneficial to all of us and they would get merged quicker.

[raskyer]

The problem of this refacto it's that it's a structural refacto .. hard to push it into little commit.

When you look at the method changes, this is log method etc ... not really fundamental method.

Moreover, there's still shortcut for these method inside the former class. (Before actually develop a service container, which is on the pip).

Maybe test it ?

[elicwhite]

This PR is really far behind dev and difficult to review properly. We would still love to split this file up but would prefer doing it in many smaller, easy to understand pull requests.

@LeaklessGfy, if you are still interested in doing that, it would be much appreciated but we would understand if you don't.