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
Remove tags from lyrics #109
Conversation
@ritiek, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yask123, @sulami and @atuljangra to be potential reviewers. |
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.
@ritiek Thanks for the PR. Looks good, but can you make slight changes as I suggested in the review comments?
@@ -178,13 +178,11 @@ def query_and_download(search, has_prompts=True, is_quiet=False): | |||
except: | |||
print ('Cant get album name') | |||
try: | |||
test1 = ('') |
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.
Can we have more verbose variable names ? I know it wasn't much verbose before either, but now is a good opportunity to fix that.
test1= (test1.replace('<br/>',' ')) | ||
test1 = test1.replace('</p>',' ') | ||
for line in raw_lyrics: | ||
test1 = test1 + str(line.get_text()) + '\n' |
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.
Same
test1 = test1.replace('</p>',' ') | ||
for line in raw_lyrics: | ||
test1 = test1 + str(line.get_text()) + '\n' | ||
|
||
print (test1) |
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.
Can we remove this?
I've changed the variable names.. Does it look okay now? |
@ritiek Looks much better now. Just one more thing, can you squash all commits into one before I merge this? |
@yask123 I'm not sure but isn't it the author of the repo who chooses this at the time of merge? |
Yea @ritiek you are right. Github added this feature lately, I had forgotten about this. |
Prevents tags like
<br>, <p>, </p> ..
etc. from being present in the output lyrics.(the old code did not seem to work somehow)