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

Trying to fix the .m3u decoding problem #16

Merged
merged 8 commits into from
Feb 14, 2021
Merged

Conversation

gjelsas
Copy link
Contributor

@gjelsas gjelsas commented Nov 6, 2020

This is a easy fix for this Issue. The current Code is not storing anything in streamurl

@gjelsas gjelsas changed the title Trying to fi the m3u decoding problem Trying to fix the .m3u decoding problem Nov 6, 2020
@monofox
Copy link

monofox commented Nov 6, 2020

Are you sure it is working for all kind of variance of m3u playlists? The for loop was there for a reason. I mean if you have an advanced formatted m3u playlist, multiple first lines will have a #.
E.g.

#EXTM3U
#EXTINF:-1,SWR2
http://swr-swr2-live.cast.addradio.de/swr/swr2/live/mp3/256/stream.mp3

Tested with your changes:

[I]  22:18:32  ✔  ~/.p/gjelsas-radiorec   master  $  python3 radiorec.py record swr2 12
DEBUG:urllib3.connectionpool:Starting new HTTP connection (1): mp3-live.swr.de:80
DEBUG:urllib3.connectionpool:http://mp3-live.swr.de:80 "GET /swr2_m.m3u HTTP/1.1" 200 96
Traceback (most recent call last):
  File "radiorec.py", line 195, in <module>
    main()
  File "radiorec.py", line 192, in main
    args.func(args)
  File "radiorec.py", line 129, in record
    streamurl = tmpstr
UnboundLocalError: local variable 'tmpstr' referenced before assignment

@gjelsas
Copy link
Contributor Author

gjelsas commented Nov 12, 2020

Hmm, strange, maybe mine where resolved differently? I don't see a reason why that would be the case... I'm currently using radiorec on Ubuntu 20.04...
When debugging radiorec, the streamurl variable was empty and therefore throwing the exception described in the title if the corresponding issue...
Could this be a dependency issue for me?

@gjelsas
Copy link
Contributor Author

gjelsas commented Dec 21, 2020

I changed the code to store the HTTPResponse object in remotefile and iterate over the lines
And I added my favorite radio station...

@gjelsas
Copy link
Contributor Author

gjelsas commented Dec 22, 2020

Added the mentioned checks. Maybe writing some tests would be good in the long run...

Copy link

@monofox monofox left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution. I'd a look on it and proposed some improvements to have a better control if the application is used by an automatism / script.

radiorec.py Outdated
break
streamurl = tmpstr
if not len(tmpstr) > 1:
Copy link

Choose a reason for hiding this comment

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

Python allows to just check on if tmpstr (see https://onlinegdb.com/3tujtNGvK)

remotefile = pool.request('GET', streamurl)
except MaxRetryError:
print('The URL of the station is not found! Check' + args.station + ' in the Settings!')
sys.exit()
Copy link

Choose a reason for hiding this comment

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

Errors should be indicated with a positive number (e.g. sys.exit(1)) as this status can be checked by other scripts.

radiorec.py Outdated
try:
remotefile = pool.request('GET', streamurl)
except MaxRetryError:
print('The URL of the station is not found! Check' + args.station + ' in the Settings!')
Copy link

Choose a reason for hiding this comment

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

Errors should be indicated to stderr, not stdout. With this you're able to filter out the debug messages or info messages if you just would like to extract the errors (e.g. for sending automatically reports via scripts).
So i recommend to get a log object (via logging.getLogger(__name__)) and log it with error function.
E.g.:
logging.getLogger(__name__).error('The URL of the station is somehow faulty! Check' + args.station + ' in the Settings!')

Would do this for this line as well as line 134.

Copy link

Choose a reason for hiding this comment

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

+line 142 + line 150

radiorec.py Outdated

verboseprint(print_time() + " ... Stream URL: " + streamurl)
target_dir = os.path.expandvars(settings['GLOBAL']['target_dir'])
if not os.path.isdir(target_dir + os.sep):
Copy link

Choose a reason for hiding this comment

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

why + os.sep?
Furthermore we could (at least) try to create the directory with os.makedirs prior this check with exist_ok=True

…of the Target Directory, and some PEP8 Styling things too..
@gjelsas
Copy link
Contributor Author

gjelsas commented Dec 25, 2020

I included your mentions! Hope its okay now! Frohe Weihnachten!

@beedaddy
Copy link
Owner

@gjelsas Thanks for your contribution. For me it looks good (although I couldn't test it thoroughly).

@beedaddy beedaddy merged commit 8182cf2 into beedaddy:master Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants