-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Sniper] Fix cache, and name matching tweak #5674
Conversation
Dev Merge to Master
Respond to possible name == None Added Nidoran M/F
Respond to possible name == None Added Nidoran M/F
…monGo-Bot into gobb_snipesource
Respond to possible name == None Added Nidoran M/F
…monGo-Bot into gobb_snipesource
Previously using self.cache which gets reset every time this task is called. Moved to self.bot.sniper_cache and added check in main snipe thread to avoid unnecessary log spam.
Not required
Uses a list of uniqueid strings instead of list of dicts. Unique id is an md5 hash of (pokemon_id, latitude, longitude and expiration)
@Gobberwart, thanks for your PR! By analyzing the annotation information on this pull request, we identified @YvesHenri and @GhosterBot to be potential reviewers |
self.bot.sniper_cache.pop(0) | ||
self.bot.sniper_cache.append(uniqueid) | ||
|
||
def _build_unique_id(self, pokemon): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is totally unecessary. Use _hash instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it unnecessary? If there's an easy way to generate MD5 from string, then awesome. Please share your alternative code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you're talking about the len(self.bot.sniper_cache) thing... which is your code so you tell me why it's necessary.
name = name.replace("mr-mime","mr. mime") | ||
name = name.replace("farfetchd","farfetch'd") | ||
name = name.replace("Nidoran\u2642","nidoran m") | ||
name = name.replace("Nidoran\u2640","nidoran f") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seriously? Are you going to handle every name combination possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read the description. Obviously not, but if we KNOW about something that's causing a problem (and difflib can't handle it), then why not replace it?
return name | ||
|
||
def _get_closest_name(self, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -210,13 +227,12 @@ class Sniper(BaseTask): | |||
MIN_SECONDS_ALLOWED_FOR_CELL_CHECK = 10 | |||
MIN_SECONDS_ALLOWED_FOR_REQUESTING_DATA = 5 | |||
MIN_BALLS_FOR_CATCHING = 10 | |||
MAX_CACHE_LIST_SIZE = 200 | |||
MAX_CACHE_LIST_SIZE = 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do realize people bot on raspberry devices, right? Increasing this this high might not be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do realise that replacing dict with an md5 string makes it take FARRRRR less memory to store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But OK, sure... I'll compromise at 300. See latest commit.
@@ -232,6 +248,9 @@ def initialize(self): | |||
self.altitude = uniform(self.bot.config.alt_min, self.bot.config.alt_max) | |||
self.sources = [SniperSource(data) for data in self.config.get('sources', [])] | |||
|
|||
if not hasattr(self.bot,"sniper_cache"): | |||
self.bot.sniper_cache = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this and what's it for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You had a "self.cache" which is nice but not persistent. Every time the task runs, it reset it to []. This creates a bot-global cache which will persist through task re-inits.
|
||
# Check if already in list of pokemon we've tried | ||
uniqueid = self._build_unique_id(pokemon) | ||
if self._is_cached(uniqueid): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_snipeable should check whether its been cached or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need. It checks before it calls is_snipeable. Saves a wasted log message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_snipeable currently lumps everything together as a general fail. I've removed one of those for clarity.
OK @YvesHenri I've answered all your questions. Talk to me in chat if something is still unclear. |
Does anyone else have feedback for this? |
No feedback received and YH's requested changes are irrelevant. Merging. |
Short Description:
Two issues fixed here.