Suggestion: Add max_tries to transaction()

[haukex]

Hi, I would like to propose that the transaction() helper gets an additional argument max_tries (perhaps even with a default value) to prevent infinite loops, as can happen fairly easily by accident:

vk = Valkey()
def incr(pipe: Pipeline):
    next_value = int(pipe.get('key')) + 1
    pipe.multi()
    vk.set('key', next_value)  # WRONG!
vk.transaction(incr, 'key', max_tries=100)  # would eventually raise error instead of infinite loop

If you like this change, I can also suggest some documentation updates etc. if you like.

[mkmkme]

Hey,

Thanks for your contribution! Could you please sign off your commit? You can find how to do it here, in the failed check report.

[mkmkme]

Also I would really appreciate if you could provide a test for your changes

[haukex]

@mkmkme Thanks, I've amended and force pushed the commit to include documentation and tests. Edit: Also rebased. Edit 2: And fixed Signed-off-by. Edit 3: And a final update with a minor improvement to the tests. Done editing for now 😄

[mkmkme]

Thanks a lot @haukex !
This is a very well written PR that I am happy to approve!

@bogdanp05 @amirreza8002 could you also have a look from your side and raise your concerns if you have any? For me it looks good to be merged.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 8.16327% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.39%. Comparing base (c291080) to head (baffe45).

Files with missing lines	Patch %	Lines
tests/test_asyncio/test_pipeline.py	10.00%	18 Missing ⚠️
tests/test_pipeline.py	10.00%	18 Missing ⚠️
valkey/client.py	0.00%	5 Missing ⚠️
valkey/asyncio/client.py	0.00%	4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (c291080) and HEAD (baffe45). Click for more details.

HEAD has 303 uploads less than BASE

Flag	BASE (c291080)	HEAD (baffe45)
	305	2

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #240       +/-   ##
===========================================
- Coverage   76.28%   59.39%   -16.89%     
===========================================
  Files         129      129               
  Lines       33906    33955       +49     
===========================================
- Hits        25864    20167     -5697     
- Misses       8042    13788     +5746     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[amirreza8002]

i haven't worked with this method, but it looks ok, tho added a comment

[haukex]

After thinking about this a bit more, I think I actually prefer the updated version if max_tries and tries > max_tries: that raises an error immediately if a negative value is passed. This seems more logical to me since a negative number of tries doesn't make sense - and could even be considered worth raising a ValueError. Plus, it leaves the door open for perhaps assigning negative numbers a different meaning in the future instead of immediately assigning it the meaning "an infinite number of tries". But this is still just my opinion and I'll defer to what you prefer.