-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Update Python imports (index_definition => indexDefinition) #3490
Update Python imports (index_definition => indexDefinition) #3490
Conversation
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thank you, @petyaslavova! Much appreciated. |
@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. |
See my comment here redis/docs#1097 (comment) |
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. |
Cc: @andy-stark-redis. Andy: see Igor's comment above. |
Pull Request check-list
Please make sure to review and check all of these items:
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