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

Make redis dependency optional #869

Merged
merged 5 commits into from
Dec 26, 2022
Merged

Make redis dependency optional #869

merged 5 commits into from
Dec 26, 2022

Conversation

ahesford
Copy link

The redis package is only used when somebody uses the redis cache handler, so the imports can be restructured to make redis an optional dependency.

@ahesford
Copy link
Author

Ping

@stephanebruckert
Copy link
Member

stephanebruckert commented Oct 27, 2022

Great.

So this won't install redis by default, but is there anything that someone using redis should do to install it? Just wondering if there is anything we can document here

@Peter-Schorn
Copy link
Contributor

Peter-Schorn commented Oct 27, 2022

We could catch the import error and display a detailed error message indicating that redis is an optional dependency and must be explicitly installed (clients often expect pip install -r requirements.txt to install all the dependencies for a package).

@ahesford
Copy link
Author

For starters, I added a note to the readme in the latest commit.

@stephanebruckert stephanebruckert changed the base branch from master to v3 October 29, 2022 13:34
@stephanebruckert stephanebruckert changed the base branch from v3 to master October 29, 2022 13:35
@stephanebruckert
Copy link
Member

stephanebruckert commented Oct 29, 2022

We could catch the import error and display a detailed error message indicating that redis is an optional dependency and must be explicitly installed (clients often expect pip install -r requirements.txt to install all the dependencies for a package).

True, exceptionally for this use case of "connectors" we should go with the warning. @ahesford this is how a warning can be raised https://github.com/plamere/spotipy/blob/fa44fed76d7aecb70f9ee6cde14da5e2ce1e17d7/spotipy/client.py#L1951

For starters, I added a note to the readme in the latest commit.

Thanks that helps. Note that in any case we won't be able to merge this into v2 aka master just now, because it would break existing apps. So this will have to go into v3 https://github.com/plamere/spotipy/tree/v3
See #652
Can you please change the base of your PR from plamere:master to plamere:v3? Feel free to create a new PR if it gets messy

@ahesford ahesford changed the base branch from master to v3 October 29, 2022 14:16
@ahesford
Copy link
Author

Rebased on v3. That branch had no redis cache handler at all, so I brought the whole class over.

I added an import check during creation of a cache object to make sure redis is available and warn if not. I'm not sure there would be a good way to handle this in the functions where the import are actually needed, unless the exception catching is moved from RedisError to Exception to issue a blanket warning instead of hard errors for unexpected, non-redis failures.

try:
from redis import RedisError
except ImportError:
warnings.warn('Error importing requied module "redis"', UserWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo here: "requied".

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the word "required" may imply to some users that this module is required for the whole package. Instead, the error message should indicate that this module is required for the RedisCacheHandler.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. I also restructured the history to preserve the evolution of Redis support on the master branch and diminish the extent of my own changes.

Commit 7d23fc3
  Add RedisCacheHandler (#747)

Commit 08411b9
  Allow to set custom key in RedisCacheHandler (#761)

Commit 9a627e8
  Simplify check for existing token (#765)
@stephanebruckert stephanebruckert merged commit f005732 into spotipy-dev:v3 Dec 26, 2022
@ahesford ahesford deleted the redis branch December 30, 2022 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants