redis-py (original) (raw)

Pull Request check-list

Please make sure to review and check all of these items:

Do tests and lints pass with this change?
Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
Is the new or changed code fully tested?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Is there an example added to the examples folder (if applicable)?
Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.

Description of change

A customer tried to run some of the query_* tests and got tripped up on the import for IndexDefinition. This PR fixes those imports.

Fixes redis/docs#1097

Hi @dwdougherty, this class name was changed as part of #3469. It is related to the upcoming new release. The examples for the older versions should be inspected from the version's branches, where the breaking changes are not merged.

Hi @petyaslavova. I'm sorry, but I'm not following you. Are you saying that this PR isn't correct and can't be merged? Currently, the code examples on redis/docs are broken because of bad imports. The only way to fix this is to fix the doctests in this repo. What are you advising?

Cc: @uglide

@dwdougherty if the tests are executed with the current library code from the master branch installed locally they should not fail.
You can verify this by running 'invoke package; pip install .' in the redis-py root dir (after all dependencies are installed)- this will install the current state of the code in your Python environment.
The current file name in the examples matches the file name in the code.

This PR was the result of a community report. Is it likely that customers will use redis-py this way? I'd expect that most folks would install redis-py via the PYPI market place.

Hi @dwdougherty, You are right about that. To make the doctests compatible with the latest released library version, this change should be merged. However, we should keep in mind that the rename should be applied again after the library version containing the latest code is released.

Choose a reason for hiding this comment

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

LGTM.

@petyaslavova @dwdougherty I think this is the wrong way of fixing this. Instead, we should "pin" the version of client repos to the latest stable version.

I didn't realize these imports had already been fixed on the 5.3 branch. I can update the JSON file in our repo as you suggested, @uglide, but I'll need to be informed if it needs to change again in the future.

Imports will be changed in the future version, which has not been released yet. So the right way to keep docs aligned is to pin all client repos to the latest stable tag, so when we make any breaking changes to the master/unstable branch in client library, it doesn't change the docs. Once new stable version released, we will bump the version of the client in the docs

BTW, we need to bump client lib versions in the quickstarts anyway.