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

Hot fix for pokemon_catch_worker.py #5479

Closed
wants to merge 1 commit into from

Conversation

alexyaoyang
Copy link
Contributor

@alexyaoyang alexyaoyang commented Sep 16, 2016

Hot fix for pokemon_catch_worker.py

Fixes/Resolves/Closes (please use correct syntax):

@DBa2016 If you do something like this: catch_ncp = pokemon_config.get('catch_above_ncp', pokemon.cp_percent).
pokemon.cp_percent >= catch_ncp: is a tautology for all our users who don't have catch_above_ncp set. E.g. some users have any: {"always_catch" : true}

I've set some fall-back values for you here and changed the logic a little (catch_cp <= pokemon.cp <= catch_below_cp seems to be a contradiction, I'm not sure).

Feel free to edit them once you've found a stable fix. Please be more careful next time. :)

@mention-bot
Copy link

@alexyaoyang, thanks for your PR! By analyzing the annotation information on this pull request, we identified @DBa2016, @pokepal and @Likael to be potential reviewers

@julienlavergne
Copy link
Contributor

Why do we have default values ? If the parameter is not in configuration then the user is not interested to limit catch with these values, so they should not be taken into consideration.

@sohje
Copy link
Contributor

sohje commented Sep 16, 2016

@alexyaoyang 330L in pokemon_catch_worker.py, remove it - treats all pokemons as vip

@alexyaoyang
Copy link
Contributor Author

alexyaoyang commented Sep 16, 2016

@anakin5 I'm not sure what you mean. We are retrieving from config based on key, it is always good to have a fall back value. Sure you can add checks to see if user have that key in config or not but at the end of the day, still good to have a fall back value.

@sohje Hot fix already fixed that bug 👍

@julienlavergne
Copy link
Contributor

@alexyaoyang What I mean is that it looks like that, by default, we are not going to catch Pokemon with ncp,cp or iv below the values your set as default.

@julienlavergne
Copy link
Contributor

If I get it correctly, following configuration:

"catch": {
     "any": {"catch_above_cp": 10, "catch_above_iv": 0.1, "logic": "and" }
}

will not catch pokemon with ncp less than 0.8, which is weird since it is nowhere mentioned in my config.

@alexyaoyang
Copy link
Contributor Author

alexyaoyang commented Sep 16, 2016

@anakin5 This is a hot fix. In my opinion, it's more important to have dev branch working (currently a lot of users have all pokemon encountered are VIP), rather than a new feature which may not have as many users yet.

That's why I reverted part of his changes which broke the pokemon catch worker and told @DBa2016 to fix it as he wants, when he finds a stable fix.

@Gobberwart
Copy link
Contributor

I've done more work on this in #5494 which accounts for the default values issue as well as fixes the "is_vip" logic. Would prefer to use that instead of this as it's a more complete fix.

@alexyaoyang
Copy link
Contributor Author

@Gobberwart Awesome, closing this then 👍

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.

5 participants