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

Add "lyrics" as a command #38

Merged
merged 1 commit into from Feb 2, 2019

Conversation

Projects
None yet
2 participants
@Arsukeey
Copy link
Contributor

Arsukeey commented Jan 30, 2019

I've added the feature to show the lyrics for the song that is currently playing.
However, it adds a dependency: lyricwikia.

Sorry if you don't want more dependencies to your project, but I thought it would be cool to add this feature, because I downloaded your program for it, and I was surprised when I didn't find it.

Show resolved Hide resolved README.md Outdated
Show resolved Hide resolved spotifycli/spotifycli.py Outdated
Show resolved Hide resolved spotifycli/spotifycli.py Outdated
Show resolved Hide resolved spotifycli/spotifycli.py Outdated
Show resolved Hide resolved spotifycli/spotifycli.py Outdated
@pwittchen

This comment has been minimized.

Copy link
Owner

pwittchen commented Jan 30, 2019

Hi @Arsukeey.

Thanks for your PR and contribution. It's nice idea and we can add such functionality. Nevertheless, I don't like a few things in the code. I've added my suggestions in the code review. Moreover, please fix code formatting (you can find details in the README.md) because CI is failing and squash your commits into one.

Regards,
Piotr

@Arsukeey

This comment has been minimized.

Copy link
Contributor Author

Arsukeey commented Jan 30, 2019

Thank you for the heads-up, I was changing some variable names for testing and I forgot to remove/rename that.

@pwittchen

This comment has been minimized.

Copy link
Owner

pwittchen commented Jan 30, 2019

Thanks for the update.

There's still wrong formatting, not updated setup.py file and multiple commits. You can use rebaseandsqua.sh website for that.

@Arsukeey

This comment has been minimized.

Copy link
Contributor Author

Arsukeey commented Jan 30, 2019

Thanks for the website!
I updated the setup.py and the code now complies with pep8.

@pwittchen

This comment has been minimized.

Copy link
Owner

pwittchen commented Jan 30, 2019

Thanks,

Can you squash your 8 commits into one? I prefer to have more organized history on git (1 commit / PR). It can be done by rebasing and squashing the commits.

@Arsukeey Arsukeey force-pushed the Arsukeey:master branch 2 times, most recently from 572d17f to af325a0 Jan 30, 2019

@Arsukeey

This comment has been minimized.

Copy link
Contributor Author

Arsukeey commented Jan 30, 2019

Done! :D
But please test if it works on your computer before merging.

Show resolved Hide resolved spotifycli/spotifycli.py Outdated
Show resolved Hide resolved spotifycli/spotifycli.py Outdated
Show resolved Hide resolved spotifycli/spotifycli.py Outdated
@pwittchen

This comment has been minimized.

Copy link
Owner

pwittchen commented Jan 30, 2019

Thanks! We're almost there. I have only last small issues with the code. Please, have a look at them. Of course, I'll test it before merge ;-).

@Arsukeey Arsukeey force-pushed the Arsukeey:master branch 2 times, most recently from 5f95650 to 1143fef Jan 30, 2019

@Arsukeey

This comment has been minimized.

Copy link
Contributor Author

Arsukeey commented Jan 31, 2019

Fixed the issues.

Add lyrics
Add my spotifycli.py

Add new requirements

hotfix

add a separator in the lyrics for the sake of readability

Remove depencencies

Update spotifycli.py

Remove/Rename variables and functions, and remove unnecessary trailing whitespace

Update setup.py and spotifycli.py

fix missing comma

fix missing comma

Add lyrics

Add unicode support

@Arsukeey Arsukeey force-pushed the Arsukeey:master branch from 51e33fb to 7eb88c6 Feb 1, 2019

@pwittchen

This comment has been minimized.

Copy link
Owner

pwittchen commented Feb 2, 2019

When I execute:

./spotifycli/spotifycli.py 

I constantly get:

Traceback (most recent call last):
  File "./spotifycli/spotifycli.py", line 9, in <module>
    import lyricwikia
ImportError: No module named lyricwikia

but I've already installed lyricwikia with pip :(. Maybe something is wrong with my configuration or it work differently in a scripts like that, but I don't know how to fix it.

@Arsukeey

This comment has been minimized.

Copy link
Contributor Author

Arsukeey commented Feb 2, 2019

Have you tried to import it in the terminal? Try to pip uninstall lyricwikia and then remove the leftovers from your ~/.local/lib and /usr/local/lib. Then reinstall it and make sure you're installing for 2.7

@pwittchen pwittchen merged commit 7eb88c6 into pwittchen:master Feb 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pwittchen

This comment has been minimized.

Copy link
Owner

pwittchen commented Feb 2, 2019

Ok, after reinstalling package, it worked :-D.
PR is merged. Moreover, I've removed empty line and added exception handling when lyrics are not found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment