Skip to content
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

Feature/download progress #209

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

DSilence
Copy link
Collaborator

@DSilence DSilence commented Apr 2, 2017

Also added logs

@werwolfby werwolfby self-requested a review April 2, 2017 19:25
Copy link
Owner

@werwolfby werwolfby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this changes from #203 to #210 only

@@ -374,7 +381,7 @@ def info(self, message):
def failed(self, message, exc_type=None, exc_value=None, exc_tb=None):
if exc_value is not None:
formatted_exception = u''.join(traceback.format_exception(exc_type, exc_value, exc_tb))
failed_message = u'{0}<br/><pre>{1}</pre>'\
failed_message = u'{0}<br/><pre>{1}</pre>' \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about space? :)

self.logger.started(datetime.now(pytz.utc))
engine = Engine(self.logger, self.settings_manager, self.trackers_manager,
self.clients_manager, self.notifier_manager)
engine.execute(ids)
except:
caught_exception = sys.exc_info()[0]
log.error("An error has occurred during execute", exception=str(caught_exception))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't provide stacktrace info, may be log.error already support it?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it doesn't, as sys.exc_info()[0] returns exception type only (ref to https://docs.python.org/2/library/sys.html#sys.exc_info)
Also, it's worth to mention, that structlog has it's own event and render for handling exceptions (take a look at http://structlog.readthedocs.io/en/stable/api.html#structlog.stdlib.BoundLogger.exception)

Btw, may I ask (as not that experienced python developer) - why sys.exec_info instead of traceback.print_exception ? It's not that safe to assign it to a local variable (from docs and SO discussions).

Copy link
Owner

@werwolfby werwolfby Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We store it for next usage, not for printing right now.
And from docs:

Process event and call logging.Logger.error() with the result, after setting exc_info to True.

That was what I'm asked for, thx

@@ -175,6 +183,7 @@ def __init__(self, clients=None, default_client_name=None):
list(self.clients.values())[0] if len(self.clients) > 0 else None)

def set_default(self, name):

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎

if torrent_settings.download_dir is not None:
torrent_settings_dict['download_dir'] = torrent_settings.download_dir
client.add_torrent(base64.b64encode(torrent).decode('utf-8'), **torrent_settings_dict)
return True
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove such return True because right now if there are no exception, than it is ok :)

@@ -87,7 +90,7 @@ def test_find_torrent_without_credentials(self, transmission_client):
plugin = TransmissionClientPlugin()

torrent_hash = 'SomeRandomHashMockString'
self.assertFalse(plugin.find_torrent(torrent_hash))
assert plugin.find_torrent(torrent_hash) is False
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know it is supported syntax 👍

@@ -112,53 +103,40 @@ def add_torrent(self, torrent, torrent_settings):
client = self.check_connection()
if not client:
return False
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw exception instead in check connection directly

# Conflicts:
#	monitorrent/plugins/clients/deluge.py
#	monitorrent/plugins/clients/qbittorrent.py
#	monitorrent/plugins/clients/transmission.py
#	monitorrent/plugins/clients/utorrent.py
#	monitorrent/rest/notifiers.py
#	tests/plugins/clients/test_qbittorrent.py
#	tests/plugins/clients/test_transmission_plugin.py
#	tests/plugins/clients/test_utorrent_plugin.py
#	tests/rest/test_api_clients.py
#	tests/rest/test_api_execute.py
#	tests/rest/test_api_notifiers.py
@coveralls
Copy link

coveralls commented Apr 30, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.914% when pulling ea228e6 on DSilence:feature/download_progress into a9de167 on werwolfby:develop.

@codecov-io
Copy link

codecov-io commented May 1, 2017

Codecov Report

Merging #209 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #209    +/-   ##
=======================================
  Coverage      100%   100%            
=======================================
  Files           55     55            
  Lines         4468   4631   +163     
=======================================
+ Hits          4468   4631   +163
Impacted Files Coverage Δ
monitorrent/plugins/trackers/tapochek.py 100% <100%> (ø) ⬆️
monitorrent/engine.py 100% <100%> (ø) ⬆️
monitorrent/rest/execute.py 100% <100%> (ø) ⬆️
monitorrent/plugins/clients/utorrent.py 100% <100%> (ø) ⬆️
monitorrent/rest/new_version.py 100% <100%> (ø) ⬆️
monitorrent/plugins/clients/transmission.py 100% <100%> (ø) ⬆️
monitorrent/plugins/clients/downloader.py 100% <100%> (ø) ⬆️
monitorrent/plugin_managers.py 100% <100%> (ø) ⬆️
monitorrent/rest/notifiers.py 100% <100%> (ø) ⬆️
monitorrent/plugins/clients/deluge.py 100% <100%> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9de167...44d1f2c. Read the comment docs.

@coveralls
Copy link

coveralls commented May 1, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.914% when pulling b9c80e4 on DSilence:feature/download_progress into a9de167 on werwolfby:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 99.914% when pulling b9c80e4 on DSilence:feature/download_progress into a9de167 on werwolfby:develop.

@coveralls
Copy link

coveralls commented May 1, 2017

Coverage Status

Coverage increased (+0.002%) to 99.957% when pulling 741193d on DSilence:feature/download_progress into a9de167 on werwolfby:develop.

@DSilence
Copy link
Collaborator Author

DSilence commented May 1, 2017

Updated the pull request

@coveralls
Copy link

coveralls commented May 1, 2017

Coverage Status

Coverage increased (+0.002%) to 99.957% when pulling 44d1f2c on DSilence:feature/download_progress into a9de167 on werwolfby:develop.

@DSilence DSilence dismissed werwolfby’s stale review May 2, 2017 06:14

Fixed the issues

@werwolfby werwolfby force-pushed the develop branch 2 times, most recently from aa9da35 to 67a7ebc Compare April 6, 2023 07:37
@werwolfby werwolfby force-pushed the develop branch 2 times, most recently from ab4c80e to e42a621 Compare April 6, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants