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

Partial clean up of Python 3.7 compatibility #2928

Merged
merged 5 commits into from Jan 1, 2024

Conversation

akx
Copy link
Contributor

@akx akx commented Sep 4, 2023

Pull Request check-list

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

  • Does $ tox pass with this change (including linting)?
    • No, Tox is no longer a thing
  • 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?

Description of change

Since 5.0.0 was the last version to support Python 3.7, this PR starts cleaning up some now-unnecessary compatibility code.

@akx akx marked this pull request as ready for review September 4, 2023 10:11
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (53de308) 91.48% compared to head (78c0625) 91.49%.
Report is 1 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2928   +/-   ##
=======================================
  Coverage   91.48%   91.49%           
=======================================
  Files         129      128    -1     
  Lines       33062    33053    -9     
=======================================
- Hits        30248    30243    -5     
+ Misses       2814     2810    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

meiravgri added a commit to meiravgri/redis-py that referenced this pull request Sep 12, 2023
In RediSearch query timeout = 0 means unlimited timeout.
In the current implementation, the query timeout is only updated `if timeout` which in case of 0, translates to false.
Since the default timeout of the `query` class is None, replacing the condition with `if self._timeout is not None` will also work for 0.
If the parameter is not a positive integer, redis server will raise an exception.

related issue: redis#2928 redis#2839
dvora-h pushed a commit that referenced this pull request Sep 14, 2023
* Support query timeout = 0

In RediSearch query timeout = 0 means unlimited timeout.
In the current implementation, the query timeout is only updated `if timeout` which in case of 0, translates to false.
Since the default timeout of the `query` class is None, replacing the condition with `if self._timeout is not None` will also work for 0.
If the parameter is not a positive integer, redis server will raise an exception.

related issue: #2928 #2839

* added a test to quety.timeout(0)

* raise an exception if query TIMEOUT is a non negative integer

* fixed the query test to catach AttributeError

* Moved validating timeout to timeout()

Raise the exception when the timeout is set instead of when the query is performed

* updates test to catch the exception when query.timeout() is called

* Update redis/commands/search/query.py

Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>

* Revert "Update redis/commands/search/query.py"

This reverts commit fb2b710.

* Revert "updates test to catch the exception when query.timeout() is called"

This reverts commit 6590130.

* Revert "Moved validating timeout to timeout()"

This reverts commit 7a020bd.

* Revert "fixed the query test to catach AttributeError"

This reverts commit 25d4ddf.

* Revert "raise an exception if query TIMEOUT is a non negative integer"

This reverts commit 3fb2c68.

---------

Co-authored-by: GuyAv46 <47632673+GuyAv46@users.noreply.github.com>
@chayim chayim requested a review from dvora-h September 19, 2023 15:08
@chayim chayim added the maintenance Maintenance (CI, Releases, etc) label Sep 19, 2023
@chayim
Copy link
Contributor

chayim commented Sep 20, 2023

@dvora-h Should be part of 5.1 when this is all turfed.

@chayim
Copy link
Contributor

chayim commented Oct 5, 2023

@akx always a pleasure - thank you. This is ready for 5.1, when Python 3.7 is officially deprecated.

@chayim chayim changed the title Drop some Python 3.7 compatibility bits Partial clean up of Python 3.7 compatibilty Oct 5, 2023
@akx akx changed the title Partial clean up of Python 3.7 compatibilty Partial clean up of Python 3.7 compatibility Nov 14, 2023
@dvora-h dvora-h added the breakingchange API or Breaking Change label Jan 1, 2024
@dvora-h dvora-h merged commit aea5755 into redis:master Jan 1, 2024
43 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakingchange API or Breaking Change maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants