Refactored Credit Mining #3390
Refactored Credit Mining #3390
Conversation
egbertbouman
force-pushed
the
credit_mining_devel
branch
4 times, most recently
from
d075918 to
a72200d
Compare
egbertbouman
changed the title
| self.select_lc = self.register_task('select_torrents', LoopingCall(self.select_torrents)) | ||
| self.session_ready = Deferred() | ||
|
|
||
| def initialize(self, policies=None): |
There was a problem hiding this comment.
Maybe you could merge this method with the __init__ method, developers may forget to initialize. And, isn't "init" short for "initialize"?
|
|
||
| self.cancel_all_pending_tasks() | ||
|
|
||
| # shutil.rmtree(self.settings.save_path, ignore_errors=True) |
|
@egbertbouman is this stable and ready for review? |
egbertbouman
force-pushed
the
credit_mining_devel
branch
4 times, most recently
from
b1c64b1 to
f5dc2d3
Compare
|
@qstokkink Yep, it's ready. |
| os.makedirs(self.settings.save_path) | ||
|
|
||
| self.select_lc = self.register_task('select_torrents', LoopingCall(self.select_torrents)) | ||
| self.num_checkpoints = len(glob(os.path.join(self.session.get_downloads_pstate_dir(), '*.state'))) |
There was a problem hiding this comment.
Does this get the length of the path name?
There was a problem hiding this comment.
No, it gets the number of checkpoints that we have on disk. I use this during startup to determine when the session if ready to receive credit mining downloads.
There was a problem hiding this comment.
I really like the cleanup, good work! I have a few minor comments.
Minor comment: I usually add a line of comment above each test where I specify a bit what it exactly does. This human-readable string is visible when running the tests. On the other hand, I think the name of the test should also specify what it does so it might feel unnecessary. Anyway, it's a matter of personal preference whether to do this or not 👍
| self.add_source(source) | ||
|
|
||
| self.session_ready = Deferred() | ||
| self.session_ready.addCallback(add_sources) |
There was a problem hiding this comment.
I think this can go on one line?
| deferreds = [] | ||
| if remove_downloads: | ||
| sources = self.sources.keys() | ||
| for source in sources: |
There was a problem hiding this comment.
Maybe compress it to deferreds = [self.remove_source(source) for source in sources]?
|
|
||
| def add_source(self, source_str): | ||
| """ | ||
| Add new source into the credit mining manager |
There was a problem hiding this comment.
add a new source to the ...
| if source_str not in self.sources: | ||
| num_torrents = len(self.torrents) | ||
|
|
||
| if isinstance(source_str, basestring) and len(source_str) == 40: |
There was a problem hiding this comment.
Is there any reason to use basestring type instead of regular str?
There was a problem hiding this comment.
I used basestring since that covers unicode as well.
|
Don't forget to squash and rebase after you've answered/processed my comments :) |
egbertbouman
force-pushed
the
credit_mining_devel
branch
from
f5dc2d3 to
6b28c9c
Compare
No description provided.