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

Correctly tracking inventory changes to the cached inventory #3860

Closed

Conversation

BriceSD
Copy link
Contributor

@BriceSD BriceSD commented Aug 13, 2016

NOT INTENDED TO BE MERGED AS IT IS

I’ve made these changes from the wrong branch, only look at item_recycle_worker.py, inventory.py and recycle_items.py

Description:

With the new cached inventory becoming a core component, we can’t afford to not correctly update it after every change.

I’ve made a class (RecycleWorker) which is more or less an api wrapper.
At first I put in it the responsibility to update the inventory. While not being bad, i feel like it’s easy to misunderstand what item.remove(amount) do. Namely removing the amount from the cached inventory without making a recycling request to the server.

What I’m proposing now to recycle an item is to use item.recycle(amount). Inside this method we’re doing a call to the api and then, depending on the success of the recycle request, we’re updating the cached inventory. By doing this we’ll never change the quantity of an item from outside the class. Thus we could allow only access to Item’s attribute and deny any change from the outside. This would make the cached inventory trustable.

However this could result in a very coupled code.
Having a class that adds a level of abstraction (RecycleWorker here) seems good enough though. The item know nothing about the recycling process. Not even how is defined a succeeding call.

    def recycle(self, amount_to_recycle):
        """
        Recycle (discard) the specified amount of item from the item inventory.
        It is making a call to the server to request a recycling as well as updating the cached inventory.
        :param amount_to_recycle: The amount to recycle.
        :type amount_to_recycle: int
        :return: Returns wether or not the task went well
        :rtype: worker_result.WorkerResult
        """
        if self.count < amount_to_recycle:
            raise Exception('Tried to remove more {} than you have'.format(self.name))

        item_recycler = ItemRecycler(_inventory.bot, self, amount_to_recycle)
        item_recycler_work_result = item_recycler.work()

        if item_recycler.is_recycling_success():
            self.remove(amount_to_recycle)
        return item_recycler_work_result

I haven’t work much with the the pokemon inventory, or the pokemon catcher either. Not sure if throwing a ball during an encounter from the inventory is a good idea.
Any thoughts on the subject would be highly appreciate.

edit : @amal, @douglascamata, @TheSavior, @aeckert

@mention-bot
Copy link

@BriceSD, thanks for your PR! By analyzing the annotation information on this pull request, we identified @amal, @douglascamata and @TheSavior to be potential reviewers

@BriceSD BriceSD changed the title How to track changes to the cached inventory correctly Correctly tracking inventory changes to the cached inventory Aug 13, 2016
@douglascamata
Copy link
Member

Can you solve merge conflicts, please?

@BriceSD
Copy link
Contributor Author

BriceSD commented Aug 15, 2016

@douglascamata We should think about it more before merging imo. Having methods with same names but only some of them calling the api will confuse everyone.

@douglascamata
Copy link
Member

@BriceSD show me the code.

@k4n30
Copy link
Contributor

k4n30 commented Aug 17, 2016

@BriceSD Is this still wanting to be merged or is it being dropped for #3948

@BriceSD
Copy link
Contributor Author

BriceSD commented Aug 17, 2016

@k4n30 They don’t have the same purpose

@solderzzc
Copy link
Contributor

close since no activity in 9 days/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants