Skip to content

Commit

Permalink
[ivi] improve error detection
Browse files Browse the repository at this point in the history
  • Loading branch information
@remitamine
remitamine committed Nov 16, 2019
1 parent 6c79785 commit 9e4e864
Showing 1 changed file with 12 additions and 9 deletions.
21 changes: 12 additions & 9 deletions youtube_dl/extractor/ivi.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,17 +119,20 @@ def _real_extract(self, url):

error = video_json.get('error')
if error:
origin = error['origin']
origin = error.get('origin')
message = error.get('message') or error.get('user_message')
extractor_msg = 'Unable to download video %s'
if origin == 'NotAllowedForLocation':
self.raise_geo_restricted(
msg=error['message'], countries=self._GEO_COUNTRIES)
self.raise_geo_restricted(message, self._GEO_COUNTRIES)
elif origin == 'NoRedisValidData':
raise ExtractorError('Video %s does not exist' % video_id, expected=True)
elif origin == 'NotAllowedError':
raise ExtractorError('pycryptodome not found. Please install it.', expected=True)
raise ExtractorError(
'Unable to download video %s: %s' % (video_id, error['message']),
expected=True)
extractor_msg = 'Video %s does not exist'
elif message:
if 'недоступен для просмотра на площадке s183' in message:

This comment has been minimized.

Sign in to view

Copy link
@dstftw

dstftw Nov 16, 2019

Collaborator

There is still no direct relation between this error message returned by 3rdparty (which may easily change in future) and missing pycryptodome. Why overcomplicate this that much when it's easily possible to set flag on ImportError and ust check this flag.

This comment has been minimized.

Sign in to view

Copy link
@remitamine

remitamine Nov 16, 2019

Author Collaborator

PyCrytodome can't be imported > site s183 is used > unavailable for viewing on site s183 error message.

if we rely only on PyCryptodome not been available, then it can also change in the future.

let's assume that the s353 is no longer working and the user doesn't have PyCryptodome.

PyCrytodome can't be imported > site s183 is used > pycryptodome not found. Please install it. raised > user install PyCryptodome > extractor still fails.

This comment has been minimized.

Sign in to view

Copy link
@dstftw

dstftw Nov 16, 2019

Collaborator

And? That's not a direct relation as already said. Endpoint can make any decision to output or not output such message. If it's possible to reliably detect missing package (try to import -> success/failure) right away locally then why rely on volatile 3rdparty mechanism for that?

if we rely only on PyCryptodome not been available, then it can also change in the future.

And the probability of this change is 0 vs some much higher factor in case of non-contracted 3rdparty API.

let's assume that the s353 is no longer working and the user doesn't have PyCryptodome.

PyCrytodome can't be imported > site s183 is used > pycryptodome not found. Please install it. raised > user install PyCryptodome > extractor still fails.

So? This is perfectly legal scenario.

Much more interesting scenario is when pycryptodome is available and s353 no longer works. In this case if will fail completely though s183 may still work.

This comment has been minimized.

Sign in to view

Copy link
@remitamine

remitamine Nov 17, 2019

Author Collaborator

just to confirm that I understand you correctly.
use s353 > PyCryptodome not found > fallback to s183 > s183 doesn't work > server error not in (NotAllowedForLocation, NoRedisValidData) > raise error asking to install PyCryptodome.
otherwise:
use s353 > fails for whatever reason(timestamp request...) > fallback to s183 > s183 doesn't work > report the error from the server.

This comment has been minimized.

Sign in to view

Copy link
@dstftw

dstftw Nov 19, 2019

Collaborator

I'd stick with:

  • obtain pycryptodomex_found flag
  • pycryptodomex found path
    • Try s353:
      • If it fails for whatever reason go to Try s183 branch
      • If it succeeds: end
  • pycryptodomex not found path
    • Try s183:
      • If it fails and pycryptodomex_found is ...:
        • False ask to install pycryptodomex
        • True show whatever error s183 returned
      • If it succeeds: end

Or simply always try s183 and s353 in a row. It will cost additional request to s183 for some videos but it will keep logic much simpler and maintainable.
raise ExtractorError(
'pycryptodome not found. Please install it.',
expected=True)
extractor_msg += ': ' + message
raise ExtractorError(extractor_msg % video_id, expected=True)

result = video_json['result']
title = result['title']
Expand Down

0 comments on commit 9e4e864

Please sign in to comment.