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

Created online docs, fixed div class issue used to scrape lyrics #153

Merged
merged 23 commits into from
Aug 31, 2020
Merged

Created online docs, fixed div class issue used to scrape lyrics #153

merged 23 commits into from
Aug 31, 2020

Conversation

allerter
Copy link
Collaborator

@allerter allerter commented Aug 20, 2020

Documentation

Have a look at the docs here
Considering #63 and the need for docs, I created documentation for the library using Sphinx and also used the RTD theme for it. I used the Google style for documenting the classes, and the functions. Since the library supports all Python 3 versions, I didn't use type hints to auto-document things in Sphinx and added them manually in the docstrings. I also added some examples under the methods, and some more are in the snippets page of the docs.
I also added a picture to the index page for aesthetics. I didn't use the Genius logo, because I wasn't sure if using their logo here was allowed or no.
This is the first version of the docs, so I'd appreciate any suggestions.
To check out the built version of the docs for yourself, install the packages in the requirements file in the docs folder. And then run make html in the same directory (or just use the tox commands below).

PEP8 Check

I also did the PEP8 checks, but I'm not sure if all of them contributed to the clarity and style of the code. So feel free to revert any of those. For PEP8, I used flake8 coupled with flake8-bugbear. As for the configs I set for flake8, they're all present in the tox.ini file.

The dreaded DIV class

Looks like Genius went and changed up the class of the lyrics' div tag again (#148). I provided two solutions in the mentioned issue, but two users reported that it didn't work for them. But I can't really say anything about that since the fixes both work for me. So I added the second fix to the code.

Tox

I added tox so you can check things yourself. Running tox in the source dir of the library will run flake8, doc8 (PEP8 for docs), and create the docs to see if they are created correctly. You can run the following tox commands for the checks:

  • tox - runs flake8, doc8, and tests creating docs
  • tox -e lint - rung flake8 and doc8
  • tox -e docs tests creating docs
  • tox - test runs the tests using pytest (needs GENIUS_CLIENT_ACCESS_TOKEN env var set)

For tox, I had to edit setup.py, add some things to it, and remove reading the requirements from the text file. But if you don't want to keep the tox file, feel free to revert setup.py and remove the tox file.

…age examples, fixed div class used to scrape lyrics
@allerter allerter changed the title Created RTD-style docs, documented classes and methods, added some us… Created online docs, fixed div class issue used to scrape lyrics Aug 20, 2020
@allerter
Copy link
Collaborator Author

The home page is a little short on content. The text of Scraping song lyrics from Genius.com 🎶 could help.

lyricsgenius/api.py Outdated Show resolved Hide resolved
@johnwmillr
Copy link
Owner

@allerter, thank you so much for your work on this! I really like the additions you're making. I will make time this weekend to review your work.

@johnwmillr
Copy link
Owner

Hi @allerter, is the tox.ini file included in your changes here? I have .ini files excluded with gitignore, so you may have unknowingly not added it.

@allerter
Copy link
Collaborator Author

I wasn't aware that .ini files were excluded. The tox.ini file contains tests for checking the codes against PEP8, checking the docs against PEP8, and checking to create the docs. It can come in handy when someone else wants to make a change in those files. But you can delete it if you want to.

@johnwmillr
Copy link
Owner

Yes, I'm happy to include the tox.ini file, but I don't think you've added it to your PR. Add it with a new commit, and I'll include it.

@allerter
Copy link
Collaborator Author

My bad, now I get what you meant. I'll add it in a bit.

@johnwmillr johnwmillr marked this pull request as ready for review August 24, 2020 01:40
@allerter
Copy link
Collaborator Author

Hey. The checks passed on my Travis CI account. Looks like you haven't set the GENIUS_CLIENT_ACCESS_TOKEN in the environment variables of the repository on Travis CI's website.

@johnwmillr
Copy link
Owner

I think I haven't given PRs permission to use my token on Travis. The tests pass in my local environment. I plan to merge your PR this weekend if not before then.

@allerter
Copy link
Collaborator Author

allerter commented Aug 27, 2020

Why not give access when its value is secret?
By the way, I tried to add you as a maintainer on readthedocs.io, but I think you don't have an account there (or your username is not the same as your GitHub username). If you'd like to be added, please let me know.

Copy link
Owner

@johnwmillr johnwmillr left a comment

Choose a reason for hiding this comment

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

Why not give access when its value is secret?
By the way, I tried to add you as a maintainer on readthedocs.io, but I think you don't have an account there (or your username is not the same as your GitHub username). If you'd like to be added, please let me know.

I checked, and Travis-CI actually doesn't allow PRs from external repositories as this prevents possible security issues, even if the credentials are encrypted.

I created an account on readthedocs.io using my GitHub username, so you should be able to add me there now.

@allerter, I really appreciate the additions you've made to the library and the time you've spent helping users who raise issues and open PRs. Would you be interested in taking on a role as a maintainer for LyricsGenius? I think you could play a substantial role in keeping the library up to date and introducing valuable new features. Send me an email if you're interested.

John

lyricsgenius/api.py Outdated Show resolved Hide resolved
@allerter
Copy link
Collaborator Author

I added you on ReadTheDocs. You should have access now.
I'd be glad to take part in maintaining the library and have sent you an email.

Copy link
Owner

@johnwmillr johnwmillr left a comment

Choose a reason for hiding this comment

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

Looks great! I will merge the PR now.

@johnwmillr johnwmillr merged commit 03c8d06 into johnwmillr:master Aug 31, 2020
@allerter allerter deleted the add-documentation branch September 1, 2020 22:24
@allerter allerter linked an issue Sep 6, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more usage examples and documentation
2 participants