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

Update repr of important classes with module name and recommended "< … #3001

Merged
merged 3 commits into from Dec 11, 2023

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Oct 12, 2023

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

the __repr__ of important classes was missing the name of the module, which is often important for
disambiguation. Also, the format should typically be "< ... >", according to the documentation, for consistency.
Update repr and tests accordingly

The motivation for this is to get more informative ResourceWarning errors, e.g. via PR #2999

@codecov-commenter
Copy link

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (5391c5f) 91.36% compared to head (d2d7304) 77.29%.

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

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3001       +/-   ##
===========================================
- Coverage   91.36%   77.29%   -14.08%     
===========================================
  Files         126      126               
  Lines       32617    32618        +1     
===========================================
- Hits        29802    25212     -4590     
- Misses       2815     7406     +4591     
Files Coverage Δ
redis/asyncio/client.py 92.13% <100.00%> (ø)
redis/asyncio/connection.py 82.16% <100.00%> (-1.50%) ⬇️
redis/connection.py 79.11% <100.00%> (-3.21%) ⬇️
tests/test_asyncio/test_connection_pool.py 92.03% <100.00%> (+0.01%) ⬆️
tests/test_connection_pool.py 100.00% <100.00%> (ø)
redis/client.py 91.67% <0.00%> (-0.31%) ⬇️
redis/asyncio/sentinel.py 83.92% <0.00%> (ø)
redis/sentinel.py 86.95% <0.00%> (ø)

... and 30 files with indirect coverage changes

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

@kristjanvalur kristjanvalur marked this pull request as ready for review October 12, 2023 15:59
@chayim
Copy link
Contributor

chayim commented Oct 16, 2023

@dvora-h I think this is a 5.1 change if it's the sort of thing we want to consider.

@chayim chayim added the breakingchange API or Breaking Change label Oct 16, 2023
@kristjanvalur
Copy link
Contributor Author

Sure. the changes are basically:

  1. give the module name as well as the class, to avoid ambiguity.
  2. Put the entire thing in <> as is the Python convention: https://docs.python.org/3/reference/datamodel.html?highlight=__repr__#object.__repr__

The reason I touched this was to make sure that any ResourceWarning errors would provide as useful info as possible.

@dvora-h dvora-h added the maintenance Maintenance (CI, Releases, etc) label Dec 11, 2023
@dvora-h dvora-h merged commit 50245cd into redis:master Dec 11, 2023
53 of 54 checks passed
@kristjanvalur kristjanvalur deleted the kristjan/repr branch December 11, 2023 08:17
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