Skip to content

Say non-negative in the shared positive-count range error message#4184

Open
AlisinaDevelo wants to merge 1 commit into
valkey-io:unstablefrom
AlisinaDevelo:fix-non-negative-count-message
Open

Say non-negative in the shared positive-count range error message#4184
AlisinaDevelo wants to merge 1 commit into
valkey-io:unstablefrom
AlisinaDevelo:fix-non-negative-count-message

Conversation

@AlisinaDevelo

Copy link
Copy Markdown
Contributor

getPositiveLongFromObjectOrReply validates against [0, LONG_MAX] but its default error says "value is out of range, must be positive", which reads as excluding 0 — yet 0 is accepted:

> RPUSH k a b c
(integer) 3
> LPOP k 0
(empty array)
> LPOP k -1
(error) ERR value is out of range, must be positive

All five call sites of the default message (LPOP/RPOP, SPOP, ZPOPMIN/ZPOPMAX, DEBUG keys/valsize) accept a non-negative count, so the wording is wrong for every one of them. Change the shared default to "must be non-negative" — one change, consistent everywhere.

Updated the three ZPOPMIN/ZPOPMAX assertions in zset.tcl that pinned the old wording. list.tcl matches loosely on *ERR*range* and set.tcl has no message assertion, so both are unaffected; the count=0 tests still pass. The stream ENTRIESREAD/ENTRIESADDED messages are separate literals (#4145/#4146) and out of scope.

Reported by @power-edge. Fixes #4150.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9be9bf0f-fe13-4d88-8942-cd4d1793ee8b

📥 Commits

Reviewing files that changed from the base of the PR and between 034b94b and 7035710.

📒 Files selected for processing (2)
  • src/object.c
  • tests/unit/type/zset.tcl
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/object.c
  • tests/unit/type/zset.tcl

📝 Walkthrough

Walkthrough

The shared positive-long validation message now says “must be non-negative” to match its [0, LONG_MAX] range. ZPOPMIN and ZPOPMAX tests were updated for negative counts across multiple setup scenarios.

Changes

Count validation wording

Layer / File(s) Summary
Update validation wording and assertions
src/object.c, tests/unit/type/zset.tcl
The shared validation helper now reports non-negative values, and ZPOPMIN/ZPOPMAX negative-count tests expect the revised error message.

Estimated code review effort: 1 (Trivial) | ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the main change: replacing the shared error text with a non-negative message.
Description check ✅ Passed The description matches the patch and explains the same error-message wording fix and test updates.
Linked Issues check ✅ Passed The code and test changes satisfy #4150 by correcting the shared range message and updating the affected assertions.
Out of Scope Changes check ✅ Passed No unrelated code changes are shown beyond the shared message fix and matching zset test updates.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Signed-off-by: AlisinaDevelo <Alisinakarimi.2003@gmail.com>
@AlisinaDevelo
AlisinaDevelo force-pushed the fix-non-negative-count-message branch from 034b94b to 7035710 Compare July 16, 2026 21:21
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.83%. Comparing base (79fc841) to head (7035710).

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable    #4184   +/-   ##
=========================================
  Coverage     76.83%   76.83%           
=========================================
  Files           162      162           
  Lines         81455    81455           
=========================================
+ Hits          62585    62588    +3     
+ Misses        18870    18867    -3     
Files with missing lines Coverage Δ
src/object.c 92.36% <100.00%> (+2.00%) ⬆️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] "must be positive" error for count arguments that accept 0 (shared getPositiveLongFromObjectOrReply)

1 participant