-
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
Fix step walker and move to fort issues #5407
Conversation
@anakin5, thanks for your PR! By analyzing the annotation information on this pull request, we identified @douglascamata, @TheSavior and @Gobberwart to be potential reviewers |
return WorkerResult.SUCCESS | ||
if self.destination is not None: | ||
self.walker = walker_factory(self.config_walker, self.bot, self.destination['latitude'], self.destination['longitude']) | ||
self.walker.is_arrived = self.walker_is_arrived |
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 are monkeypatching walker is_arrived method? fine by me if documented 👍
But I wonder how clear is this for everybody...
Could you please at least add a comment in the code...!
(Raise you hands whoever does not know what monkey patching is! :) )
http://cosent.nl/en/blog/monkey-patching-good-bad-ugly
We would be probably be better off by adding a parameter to the StepWalker With a usage something like: That could benefit to other tasks also.. not monkeypatch them all one by one...! |
@anakin5 is this still a change you want to do? I see it now detached from your repo/branch |
it got detached when this repo has been deleted. I still plan to do the changes and create another pr |
@anakin5 ping? |
@davidakachaos Do you mind merging this to your changes? So that when both yours and this are committed, I can move on to investigate what causes walker not moving after a session slate. As this PR prevent polyline from moving between 2 forts continuously. |
@MerlionRock I don't mind, but my two PRs were merged already. And I didn't modify these files, so could you please tell me where I should merge? |
@davidachaos then maybe I'll try to raise a PR myself base on what has already being merged. Just not sure how to correct that Monkey patching.... |
@MerlionRock Okay, well, if I can help you out in some way, let me know 😄 |
@davidakachaos Step walker has no conflict, I'll do a PR for it. I've changed move_to_fort with a little simple logic to prevent it from infinitely switching between 2 forts instead. Will merge it instead of using Anakin5's. But I'll wait for the 0.53 API to be ready first before doing these PR. |
Cool. Now we wait for a stable api again. Hope it will be available soon.
Op 21 jan. 2017 04:56 schreef "MerlionRock" <notifications@github.com>:
… @davidakachaos <https://github.com/davidakachaos> Step walker has no
conflict, I'll do a PR for it.
I've changed move_to_fort with a little simple logic to prevent it from
infinitely switching between 2 forts instead. Will merge it instead of
using Anakin5's.
But I'll wait for the 0.53 API to be ready first before doing these PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5407 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD5Y-WgzVuK1Bxern_djmAtDRSiB7Hyks5rUYHbgaJpZM4J6jSp>
.
|
* Proto requirements fix (#5888) * Added Total Stardust to UpdateLiveStats task (#5874) * added Total Stardust to UpdateLiveStats task * fixes to tests, thanks to @nelsyeung * enabled option to log to file (#5881) * Fixes #5883 (#5884) This fixes an issue when you have a brand new account (no stardust yet) and run the bot. This makes sure the key is there before we do something with it. * removing proto and protobuf and changing pogoapi to dev * removing proto and protobuf, changing pogoapi to dev branch, fixing docs and install scripts * Prevent bouncing between 2 forts Added simple logic to prevent bouncing between 2 forts when using Polyline walker * Original fixes by Anakin5 Original PR by Anakin5 which fixes the following: #5407 Fix step walker timing issue. This solve some problems where we were moving more than the configured speed from time to time. Improve a bit the computation speed of step_walker get_next_step pep8 fixes * Prevent bouncing between 2 forts Added simple logic to prevent bouncing between 2 forts when using Polyline walker
@MerlionRock I think this PR can be closed now? cc @solderzzc |
@davidakachaos I guess so. If @anakin5 ever return, he can reopen it. cc @solderzzc |
Yes, Let me close it by now. @davidakachaos @MerlionRock |
Will fix #5384 but potentially other weird behavior as well