Optimize ping() method

[spyinx]

ping() method in ManagementCommands

valkey-py/valkey/commands/core.py

Lines 1203 to 1209 in c490780

	def ping(self, **kwargs) -> ResponseT:
	"""
	Ping the Valkey server
	For more information see https://valkey.io/commands/ping
	"""
	return self.execute_command("PING", **kwargs)

when i use ping() method in class ManagementCommands, i have some questions：

• it does't support message param, but ping() in class Pubsub does, why?
  

  valkey-py/valkey/client.py

  Lines 1079 to 1084 in c490780

  	def ping(self, message: Union[str, None] = None) -> bool:
  	"""
  	Ping the Valkey server
  	"""
  	args = ["PING", message] if message is not None else ["PING"]
  	return self.execute_command(*args)

• ping() method has kwargs to allow extra options, but does this method really need it? if you add some extra options, it will raise a exception because of the callback in _ValkeyCallbacks

>>> from valkey import Valkey
>>> r = Valkey(host="127.0.0.1", port=6888, password='xxxxx')
>>> r.ping()
True
>>> r.ping("hello")
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    r.ping("hello")
    ~~~~~~^^^^^^^^^
TypeError: ManagementCommands.ping() takes 1 positional argument but 2 were given
>>> r.ping(message="hello")
Traceback (most recent call last):
  File "<python-input-4>", line 1, in <module>
    r.ping(message="hello")
    ~~~~~~^^^^^^^^^^^^^^^^^
  File "/root/.pyenv/versions/env-3.13/lib/python3.13/site-packages/valkey/commands/core.py", line 1211, in ping
    return self.execute_command("PING", **kwargs)
           ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/root/.pyenv/versions/env-3.13/lib/python3.13/site-packages/valkey/client.py", line 570, in execute_command
    response = conn.retry.call_with_retry(
        lambda: self._send_command_parse_response(
    ...<2 lines>...
        lambda error: self._disconnect_raise(conn, error),
    )
  File "/root/.pyenv/versions/env-3.13/lib/python3.13/site-packages/valkey/retry.py", line 62, in call_with_retry
    return do()
  File "/root/.pyenv/versions/env-3.13/lib/python3.13/site-packages/valkey/client.py", line 571, in <lambda>
    lambda: self._send_command_parse_response(
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        conn, command_name, *args, **options
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ),
    ^
  File "/root/.pyenv/versions/env-3.13/lib/python3.13/site-packages/valkey/client.py", line 543, in _send_command_parse_response
    return self.parse_response(conn, command_name, **options)
           ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/.pyenv/versions/env-3.13/lib/python3.13/site-packages/valkey/client.py", line 600, in parse_response
    return self.response_callbacks[command_name](response, **options)
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
TypeError: <lambda>() got an unexpected keyword argument 'message'
>>> 

Is there any historical reasons to do so? Now i do some changes for ping:

• ping():

    # in class ManagementCommands
    def ping(self, message=None, **kwargs) -> ResponseT:
        """
        Ping the Valkey server

        For more information see https://valkey.io/commands/ping
        """
        args = ["PING", message] if message is not None else ["PING"]
        return self.execute_command(*args, message=message)

• add new callback for ping command:

# import safe_str
from valkey.utils import str_if_bytes, safe_str  

def parse_ping(response, **options):
    response = str_if_bytes(response)
    message = "PONG" if options.get("message") is None else options.get("message")
    return response == safe_str(message)


_ValkeyCallbacks = {
    # ...
    # "PING": lambda r: str_if_bytes(r) == "PONG",
    "PING": parse_ping,
    
    # ...
}

then we can try the new ping() method:

>>> from valkey import Valkey
>>> r = Valkey(host="127.0.0.1", port=6888, password='xxxxx')
>>> r.ping()
True
>>> r.ping(0)
True
>>> r.ping("hello")
True
>>> r.ping(xxx=0)
True
>>> r.ping("abc", xxx=0)
True

[spyinx]

Maybe ping() method just use to test valkey server is alive or not, so there is no need to add extra message, but why use **kwargs to receive extra params since the default callback will occurs exception? Similar situations also include the time() method, they both use lambda expression as callback.

[aiven-sal]

Hi!
In general, we don't have a lot of insight about why certain things are done in a certain way, because we are not the original authors of most of the code in valkey-py. Your guess is as good as mine here.

Your proposal makes sense to me.

But I'd rewrite

 message = "PONG" if options.get("message") is None else options.get("message")

as

 message = options.get("message", "PONG")

[aiven-sal]

Would you like to make a PR?

[spyinx]

Hi! In general, we don't have a lot of insight about why certain things are done in a certain way, because we are not the original authors of most of the code in valkey-py. Your guess is as good as mine here.

Your proposal makes sense to me.

But I'd rewrite

 message = "PONG" if options.get("message") is None else options.get("message")

as

 message = options.get("message", "PONG")

there is a problem using message = options.get("message", "PONG"), because options.get("message") will get None when message is None, it will not be "PONG":

>>> data = {"message": None}
>>> data.get("message", "PONG") == None
True

[spyinx]

Would you like to make a PR?

i will make a pr, thanks for your reply, i'll continue to fix real bugs in valkey-py and try to make it better. now i only read valkey's command methods, so my opinions may not be good or even wrong, but i will think more before asking questions.

[spyinx]

it was my fault to think that this issue was easy to fix, i didn't understand the code well enough. adding a message that would affect the callback of the cluster connection in ping. Although I found the callback code , I thought it was unnecessary to modify both, so I closed my pr. I will test more cases before creating a pr in the future. thanks again @aiven-sal

[aiven-sal]

No problem!