-
Notifications
You must be signed in to change notification settings - Fork 452
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
Refactor magnet link copy function #8017
Conversation
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 have a mixed feeling about this PR.
First, it looks like the actual problem still needs to be fixed. If I understand correctly, the reason for the TypeError: 'NoneType' object is not subscriptable
is that the field self.current_download
is None.
At the beginning of other methods of the DownloadsDetailsTabWidget
, we can see the following checks:
def update_pages(self, new_download=False):
if self.current_download is None:
return
...
It looks to me that a similar check should be added to the beginning of the on_copy_magnet_clicked
method.
Second, the resulting code after the PR refactoring does not look clearer than the original code. Originally, we had a 9-line function, and now we have two functions, 32 lines in total, that do the same, just in a more verbose form. Furthermore, now a future code developer should deal with two very similar functions, create_magnet
and compose_magnetlink
. It may be hard for a developer to understand, which one of these functions should be used in which case.
Can we instead add filtering valid tracker urls directly to the compose_magnetling
function, completely avoiding the create_magnet
static method?
Yes, it is more verbose, but that is not necessarily a bad thing. More lines of code do not always equate to worse code. It is more verbose because:
trackers = [
tk['url'] for tk in self.current_download['trackers'] if 'url' in tk and tk['url'] not in ['[DHT]', '[PeX]']
] was rewritten to: invalid_urls = {'[DHT]', '[PeX]'}
urls = (t.get('url') for t in trackers)
valid_urls = [u for u in urls if u and u not in invalid_urls] This makes it more understandable by splitting a large and complex expression into two expressions with clear purposes:
|
@kozlovsky, thank you for the review. I've addressed most of your comments except for the verbosity. For the explanation, please see above. |
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.
@drew2a, thank you for addressing the comments; now the code looks clear and understandable!
The function for copying the magnet link has been refactored. The code now is a bit safier.
Fixes #8016