Changed cache_handler.py to utilize Python's Context Management Protocol #991
Conversation
… to utilize pythons context management protocol to open and close files, instead of opening and closing manually. More information specified in the pull request.
|
updated change log |
spotipy/cache_handler.py
Outdated
| with open(self.cache_path) as f: | ||
| token_info_string = f.read() | ||
| token_info = json.loads(token_info_string) | ||
| except IOError as error: |
There was a problem hiding this comment.
Let's try to also use OSError as it's more general and can catch more issues.
The master branch is currently using OSError https://github.com/spotipy-dev/spotipy/blob/2.25.0/spotipy/cache_handler.py#L84 since #1065
There was a problem hiding this comment.
Hey! I see the issue with the mock object not supporting the context manager. I'll also swap back to OSError.
Working on the Mock updates now and will test locally. I’ll push the changes once everything is working.
Thank you!
* Update docs Add maximum value for limit parameter. See: https://developer.spotify.com/documentation/web-api/reference/get-users-top-artists-and-tracks * Update CHANGELOG.md --------- Co-authored-by: Stéphane Bruckert <[email protected]>
… to utilize pythons context management protocol to open and close files, instead of opening and closing manually. More information specified in the pull request.
|
Made some updates to the unit tests. The main issue was incorrect handling of the open function, which didn’t support the context manager protocol when mocked. Let me know what you think and/or if there is anything else needed from my end. |
|
Thanks for the quick updates! Tests ran but seem to be failing, please can you check again? 🙏 I've now relaxed the rule that prevented to run the github action workflow for first-time contributors, so if you push, you will be able to see if it passes directly. |
|
Hi! Sorry about that. Thank you for looking into it. Yes I am checking again and will ensure the tests pass. |
… to utilize pythons context management protocol to open and close files, instead of opening and closing manually. More information specified in the pull request.
* Update docs Add maximum value for limit parameter. See: https://developer.spotify.com/documentation/web-api/reference/get-users-top-artists-and-tracks * Update CHANGELOG.md --------- Co-authored-by: Stéphane Bruckert <[email protected]>
… to utilize pythons context management protocol to open and close files, instead of opening and closing manually. More information specified in the pull request.
…into 508chris-branch
|
Tests still not passing (I modified the workflows again so we can see the results directly, hopeful they always run now). I also got a version working for the file context manager and mocking, feel free to copy everything from it master...context-manager-mock |
|
Apologies for the confusion — I was pushing and syncing changes to my forked version while testing the workflow, but I haven't finalized all the fixes yet. I’ll review the updates in your version to see how they compare with my current progress. Thank you for staying on top of this and for the helpful pointers. I’ll finalize my changes by tomorrow or the day after and push everything again for review. |
|
np, thanks! |
|
Hey, I am testing out your context-manager-mock branch and getting fails/errors locally. The failures are due to (I believe) hitting spotify api limits. Also seeing failures for SSL errors. Have you experienced anything similar? |
Updated integration tests to mock `_get` and `_post` methods in the Spotify class
|
Actually - all tests passed on my fork when I pushed some changes. I believe the tests are failing on the upstream repository side as I do not have access to configure the required secrets for the workflow. Let me know what you think. Thank you! |
That's right! I've merged it and it all passed https://github.com/spotipy-dev/spotipy/actions/runs/12919090082 Awesome stuff thank you for jumping on this again after a year! |
|
I only added those mocks as I was getting errors on test_add_to_queue tests due to "missing 1 required positional argument: 'mock_post'". Might've been missing something sorry bout that. Thank you for looking into this as well as working with me on it. Learned a lot and very appreciative of the opportunity to contribute. Thanks again! |
I changed two methods within the CacheHandler class, get_cached_token and save_token_to_cache. Instead of opening and closing files manually, I updated these methods to use Python's "with" statement to keep up with good practice in Python programming.
This will also alleviate any errors that occur before the file is closed which would result in allocated resources.
Using the with statement, it simplifies the code as well as provides more reliability to the code base.
I also added an except clause to the get_cached_token method to handle any json decode errors.
Let me know if you have any questions at all.