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

Add spotify as a fetchart source #4771

Merged
merged 9 commits into from
Apr 30, 2023
Merged

Conversation

arsaboo
Copy link
Contributor

@arsaboo arsaboo commented Apr 27, 2023

Added spotify as a fetchart source.

Let me know if this approach looks fine.

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

@wisp3rwind
Copy link
Member

Looks reasonable at a first glance 🎉 (Without checking anything Spotify-specific.) One small thing: We list beautifulsoup as an optional dependency, so we need to import in conditionally. See for example the lyrics plugin and the dependency specification in setup.py.

@arsaboo
Copy link
Contributor Author

arsaboo commented Apr 28, 2023

Thanks for the review @wisp3rwind. I have updated the code to make beautifulsoup optional and added a changelog.

@wisp3rwind
Copy link
Member

Looking good! Would you mind rebasing on (or merging) master in order to get #4772 for a proper CI run?

@arsaboo arsaboo closed this Apr 29, 2023
@arsaboo arsaboo reopened this Apr 29, 2023
@arsaboo
Copy link
Contributor Author

arsaboo commented Apr 29, 2023

Thanks @wisp3rwind for that fix...all green now ✅

@wisp3rwind
Copy link
Member

Great! I didn't check whether the Spotify "API" usage is correct, but since this is well-contained and otherwise the code looks good, I'm just going ahead and merge.

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