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

Removed Redis parser_class reference from docs. #18052

Merged
merged 1 commit into from May 8, 2024
Merged

Conversation

r3a70
Copy link
Contributor

@r3a70 r3a70 commented Apr 5, 2024

Trac ticket number

ticket-[number]

Branch description

Provide a concise overview of the issue or rationale behind the proposed changes.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • For UI changes, I have attached screenshots in both light and dark modes.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

@shangxiao
Copy link
Contributor

Interesting. redis-py still has a parser_class option but the classes seem to have been made private from 4.4.x onwards as they are all prefixed with "_" and the parsers section was removed from the README.md.

I don't think it's helpful to remove the hyperlink, rather someone with more knowledge could suggest a correct link.

@r3a70
Copy link
Contributor Author

r3a70 commented Apr 5, 2024

unfortunately there is not link that describe the parser_class (i found one but it's for another third-party lib)

@r3a70
Copy link
Contributor Author

r3a70 commented Apr 5, 2024

I think it's okay to remove it when redis-py removes the documentation for the parser classes. The redis-py documentation says that redis-py automatically uses the hiredis-py parser class if you install hiredis-py. This tells us that we have two options:

  1. Install it and let redis use it automatically.
  2. Don't install it and let redis use its own parser.

@@ -563,7 +563,7 @@ flag on the connection's socket::

Here's an example configuration for a ``redis`` based backend that selects
database ``10`` (by default Redis ships with 16 logical databases), specifies a
`parser class`_ (``redis.connection.HiredisParser`` will be used by default if
parser class (``redis.connection.HiredisParser`` will be used by default if
Copy link
Member

Choose a reason for hiding this comment

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

Using a custom parser class is so niche that it's not even documented in redis-py docs. I'd remove it:

diff --git a/docs/topics/cache.txt b/docs/topics/cache.txt
index 3d67f9b03a..1fe9d335fb 100644
--- a/docs/topics/cache.txt
+++ b/docs/topics/cache.txt
@@ -562,10 +562,8 @@ flag on the connection's socket::
     }
 
 Here's an example configuration for a ``redis`` based backend that selects
-database ``10`` (by default Redis ships with 16 logical databases), specifies a
-`parser class`_ (``redis.connection.HiredisParser`` will be used by default if
-the ``hiredis-py`` package is installed), and sets a custom `connection pool
-class`_ (``redis.ConnectionPool`` is used by default)::
+database ``10`` (by default Redis ships with 16 logical databases), and sets a
+custom `connection pool class`_ (``redis.ConnectionPool`` is used by default)::
 
     CACHES = {
         "default": {
@@ -573,13 +571,11 @@ class`_ (``redis.ConnectionPool`` is used by default)::
             "LOCATION": "redis://127.0.0.1:6379",
             "OPTIONS": {
                 "db": "10",
-                "parser_class": "redis.connection.PythonParser",
                 "pool_class": "redis.BlockingConnectionPool",
             },
         }
     }
 
-.. _`parser class`: https://github.com/redis/redis-py#parsers
 .. _`connection pool class`: https://github.com/redis/redis-py#connection-pools
 
 .. _the-per-site-cache:

Copy link
Member

Choose a reason for hiding this comment

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

@r3a70 Do you have time to keep working on this?

Copy link
Contributor Author

@r3a70 r3a70 Apr 30, 2024

Choose a reason for hiding this comment

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

@felixxm can you explain what i should be to do ?

Copy link
Member

Choose a reason for hiding this comment

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

I left a diff with a suggestion in my previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i work on it.

@sarahboyce sarahboyce self-assigned this May 7, 2024
@sarahboyce sarahboyce changed the title Deleted [broken] url from doc. Removed Redis parser_class reference from docs. May 7, 2024
Copy link
Contributor

@sarahboyce sarahboyce left a comment

Choose a reason for hiding this comment

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

Thank you for this @r3a70

@sarahboyce sarahboyce merged commit 0e445ba into django:main May 8, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants