aio-libs / aiomcache

Minimal asyncio memcached client
BSD 2-Clause "Simplified" License
141 stars 38 forks source link

Extra loop join generate command #403

Closed ArtemIsmagilov closed 5 months ago

ArtemIsmagilov commented 5 months ago

try use more simple code with literals(remove join and list comprehension) python use by default encoding utf-8

Dreamsorcerer commented 5 months ago

I'm not so sure this one is an improvement (especially when introducing legacy % string formatting).

I didn't write any of this code originally, but if I was to rewrite it, I'd just lose the lists:

args = (str(a).encode() for a in (flags, exptime, len(value)))
_cmd = b" ".join((command, key, *args))
ArtemIsmagilov commented 5 months ago

@Dreamsorcerer. Hi, thanks for your comment. Let me explain why I decided to rewrite the code this way.

  1. Using % is not deprecated for byte strings and is the only way to format byte strings literally. Even if you confuse with strings, formatting using % is indeed an old style, but not outdated.
  2. you encode each element separately in a loop and then connect it separately. I suggested making this entire part in one line, using the arguments at once.
  3. However, I really can’t refute the fact that perhaps this is how you explicitly highlight for the developer that these are the arguments of the args command
Dreamsorcerer commented 5 months ago

OK, I reminded myself about bytes interpolation today. I think I'd be fine with your original idea if you dropped the f-strings and used b"%b %b %a %a %a" to format it in one step (note that %s is actually deprecated and likely won't work in Python 4).

ArtemIsmagilov commented 5 months ago

OK, I reminded myself about bytes interpolation today. I think I'd be fine with your original idea if you dropped the f-strings and used b"%b %b %a %a %a" to format it in one step (note that %s is actually deprecated and likely won't work in Python 4).

Thank you for considering this issue and reminding us about the outdated %s. I will finish the PR.