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

Help users know what's wrong if they make the natural mistake of using a Spotiify URI instead of a Spotify link #138

Merged
merged 3 commits into from
Jan 21, 2021

Conversation

pbx
Copy link
Contributor

@pbx pbx commented Jan 21, 2021

Thanks for the software! I'm offering this PR as a possible UX improvement.

I made this mistake when I was trying out this app for the first time today. Running current master (44cf2d3), here's the output you get if you provide a Spotify URI like spotify:playlist:0wMH0M981nj70CQLzF1CzJ instead of a "Spotify link" as they call them:

Starting spotify_dl
Traceback (most recent call last):
  File ".../bin//spotify_dl", line 8, in <module>
    sys.exit(spotify_dl())
  File ".../spotify_dl/spotify_dl.py", line 69, in spotify_dl
    valid_item = validate_spotify_url(args.url)
  File ".../spotify_dl/spotify.py", line 90, in validate_spotify_url
    item_type, item_id = parse_spotify_url(url)
  File ".../spotify_dl/spotify.py", line 64, in parse_spotify_url
    item_id = parsed_url.split("/")[1]
IndexError: list index out of range
Sentry is attempting to send 0 pending error messages
Waiting up to 2 seconds
Press Ctrl-C to quit

I figured a one-line error message would be more helpful and friendly.

@pbx
Copy link
Contributor Author

pbx commented Jan 21, 2021

Hrm, at first read-through I'm not sure why this change is causing those test failures. I'll check it out if I get some more free time.

@SathyaBhat
Copy link
Owner

Hi @pbx thanks for the PR! The tests pass locally, I think its more of something to do with spotify rejecting credentials when coming in from Github actions - the same creds work locally. I'll try to take a look at it in a separate PR, for now I will merge it.

Thanks again for your contribution

@SathyaBhat SathyaBhat merged commit 1e083d8 into SathyaBhat:master Jan 21, 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.

2 participants