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

abandon-lured-pokemons-when-not-enough-pokeball #4520

Closed
wants to merge 1 commit into from
Closed

abandon-lured-pokemons-when-not-enough-pokeball #4520

wants to merge 1 commit into from

Conversation

leanhdaovn
Copy link
Contributor

  • Abandon lured pokemons when there's not enough pokeball
  • Refactor code for checking remaining pokeball quantity

@mention-bot
Copy link

@leanhdaovn, thanks for your PR! By analyzing the annotation information on this pull request, we identified @pokepal, @xc-mezcal and @RedViper9 to be potential reviewers

@mjmadsen
Copy link
Contributor

mjmadsen commented Aug 22, 2016

Note commit 695ca70 no longer uses catch_visible_pokemon.py and catch_lured_pokemon.py. There is also a ball check. But now that I say that, I don't know if we're updating balls after attempts.

@leanhdaovn
Copy link
Contributor Author

I think that commit works just fine @mjmadsen.
Only the edge case of min_ultraball_to_keep for vip pokemons hasn't been handled.

@k4n30
Copy link
Contributor

k4n30 commented Aug 23, 2016

@leanhdaovn not only is there merge conflicts that needs resolving, but you have made significant changes to base task which IMHO is bad as no other tasks that are pulling from base task need these changes and will need testing against all those who inherit from base task.

You should move all your changes to the function of which you are trying to fix, which is in this case the "catch pokemon" worker which merged catch lured and catch visible pokemon recently.

@Quantra Are you planning on doign the rest of the deprecation of catch lured, and catch visible files as they are going to cause issues. My suggestion is to fully deprecate them, rather than allow users to use both options depending on their config, as changes will need to be made to both options

@leanhdaovn
Copy link
Contributor Author

@k4n30 I think we can close this PR since the commit @mjmadsen mentioned above addressed the problem

@leanhdaovn leanhdaovn closed this Aug 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants