-
Notifications
You must be signed in to change notification settings - Fork 30
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
Added some feature requests #7
Conversation
625d357
to
88e744e
Compare
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.
Thank you for the initiative.
I've made some notes.
else: | ||
str_type = basestring | ||
byte_types = bytes | ||
chr_ = lambda ch: ch | ||
int_types = (int, long) |
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 one will be merged in #4.
@@ -0,0 +1,444 @@ | |||
from os.path import join, isdir, getsize, normpath, basename, getsize |
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.
A new file - hard to see was have changed, and what is new.
Classic approach is to make several braches (based on one another is they are connected) with gradual improvements introduction.
|
||
_filepath = None | ||
|
||
def __init__(self, |
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.
To many params for the initializer.
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.
Id still like to keep the "settings" and remove
webseeds=None,
httpseeds=None,
private=None,
announce_list=None,
comment=None,
creation_date=None,
created_by=None
thoughts?
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.
Common approach is to encapsulate settings in some settings object, and pass that object, yet, here I hardy can see any reasons for a separate object, since most of settings do belong solely to size calculation.
|
||
return target_files_, total_size | ||
|
||
def _calc_size(self, size): |
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.
Size calculation is discussed in #5
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.
I need to keep what we have now except from the blocksize ill change that to 32kb and add hardcoded limits. torrentool will use this by default unless some of the options is set.
return self._set_announce_urls(val) | ||
|
||
@property | ||
def httpseed(self): |
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.
Feature is ok.
return 'magnet:?xt=urn:btih:' + self.info_hash | ||
|
||
@property | ||
def webseed(self): |
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.
Feature is ok.
So this is most of the feature requests I have listed in #5
Is currently missing:
Dunno if i want to add that anyway.
I have fixed all tests that was broke on windows but i havnt tried it on linux, hence the pr so travis can test it.
It still needs some clean up so i use the same docstring and comments etc, adding a test for the cli and fixing cli for windows. Im not sure how to fix it. I havnt released anything on pypi but i assume we are missing a entry point for the cli. And what ever comments you might have
I dont except the pr to be merged, but let me know what you think.
Coveralls drops since it tests the pypi package now not the pr.