-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Check and replace slashes with dashes to avoid directory creation error #402
Conversation
spotdl/internals.py
Outdated
@@ -110,6 +110,10 @@ def sanitize_title(title, ok="-_()[]{}\/"): | |||
if const.args.no_spaces: | |||
title = title.replace(" ", "_") | |||
|
|||
# replace slashes with "-" to avoid folder creation errors | |||
if isinstance(title, str): |
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 don't think we need this check here. The function sanitize_title
already expects title
to be a string.
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.
The reason I did that, was because the Travis Build failed because I was trying to call .replace()
on an int
title. Maybe there's an issue with the test?
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 think the issue is here:
IMO this should be sanitize_title(str(format_tags[tag]), ok="'-_()[]{}")
, like two lines below.
Also the code is rather unpythonic:
Iterating over a dict
like this is effectively iterating over its keys. Instead of doing this and looking up the value manually (format_tags[tag]
), we should iterate over format_tags.values()
. Changing a dict while iterating is bad anyways: https://cito.github.io/blog/never-iterate-a-changing-dict/
We might as well use a dict comprehension here:
format_tags_sanitized = {
k: sanitize_title(str(v), ok="'-_()[]{}")
if slugification
else str(v)
for k, v in format_tags.items()
}
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.
Ok, this seems like a better fix than I proposed so I'll add it in and see if it works!
I don't know why this is failing, I will check it out tomorrow as I am busy today. |
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 re-ran the travis build and it seems to have passed this time. Going to merge this in. Thanks!
A potential solution to #321
Since the
sanitize_title
function ofinternals.py
handles the filename generation, I decided to add a line replacing forward and backslashes with simple dashes there.I don't know if this was the solution you were looking for, but let me know if there's a better way!