Skip to content

Fixed outdated testcases and all compiler warning issues #1662

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

srinivas365
Copy link

@srinivas365 srinivas365 commented Mar 27, 2025

Summary by CodeRabbit

  • New Features

    • Expanded command support by adding operations like PING, ECHO, SET, GET, and related key commands to enhance interaction capabilities.
    • Introduced a structured approach to testing commands against a DiceDB client, including error handling and result verification.
  • Improvements

    • Refined error messages for several commands to provide clearer, more concise feedback during command usage.
    • Adjusted the initialization process for the store across various tests to enhance configuration consistency.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

coderabbitai bot commented Mar 27, 2025

Walkthrough

The changes update multiple modules and test files to accommodate a modified dstore.NewStore signature, which now requires an additional integer parameter (set to 0). Various test files have been updated accordingly, and error messages in the Bloom filter tests have been streamlined by removing the "ERR " prefix and adjusting command names. Additionally, several new stub evaluation functions have been added to the evaluation module to expand command handling, and a new test file in the ironhawk package introduces a structured approach to executing DiceDB client commands.

Changes

File(s) Change Summary
internal/eval/bloom_test.go Updated TestBloomFilter and TestGetOrCreateBloomFilter to call dstore.NewStore(nil, nil, 0) and modified error messages by removing the "ERR " prefix and adjusting command names.
internal/eval/countminsketch_test.go
internal/eval/eval_test.go
internal/eval/hmap_test.go
internal/eval/main_test.go
internal/store/expire_test.go
Revised store initialization calls to include the new third parameter (0). In eval_test.go, removed testEvalGETSET; in hmap_test.go, updated variable assignments to ignore unused return values.
internal/store/batchevictionlru_test.go Changed instantiation by removing the eviction rate parameter in NewPrimitiveEvictionStrategy and updating NewStore calls to include an extra parameter (0).
internal/eval/eval.go Added new stub evaluation functions (evalPING, evalECHO, evalSET, evalGETEX, evalGETDEL, evalGET, evalEXPIRE, evalEXPIRETIME, evalEXPIREAT, evalTTL, evalDEL, evalTYPE, evalSETEX, evalINCR, evalINCRBY, evalDECR, evalDECRBY) to handle various commands, all currently returning nil.
tests0/main_test.go Introduced a new test file in the ironhawk package featuring a TestCase struct, a local connection function, and a test runner (runTestcases) that executes command sequences and asserts expected results.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Eval
    participant Store
    Client->>Eval: Send command (e.g., "PING", "SET", "GET")
    Eval->>Store: Process command with arguments (if applicable)
    Store-->>Eval: Return data (if applicable)
    Eval-->>Client: Return response (currently nil/stub)
Loading
sequenceDiagram
    participant T as TestRunner
    participant C as DiceDB Client
    participant DB as Server
    T->>C: Establish connection (getLocalConnection)
    T->>C: Flush database before tests
    loop For each TestCase
        T->>C: Execute command sequence
        C->>DB: Process command
        DB-->>C: Return response
        T->>C: Assert response equals expected result
    end
Loading

Poem

I'm a clever rabbit on a coding spree,
Hopping through changes as quick as can be.
New parameters and functions that gleam,
Like carrots of code in a springtime dream.
With streamlined errors and tests so bright 🥕,
I celebrate each change with a joyful hop tonight!

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (7)
internal/eval/eval.go (1)

154-156: Add stub documentation to indicate pending implementation.

These evaluation functions are currently empty stubs returning nil. To improve code maintainability and clarity, add comments indicating that these are placeholders for future implementation. Returning nil for functions that should return an *EvalResponse could lead to nil pointer dereferences if these functions are called before proper implementation.

func evalSET(args []string, store *dstore.Store) *EvalResponse {
-	return nil
+	// TODO: Implement SET command evaluation
+	return makeEvalError(diceerrors.ErrNotImplemented("SET"))
}

func evalGETEX(args []string, store *dstore.Store) *EvalResponse {
-	return nil
+	// TODO: Implement GETEX command evaluation
+	return makeEvalError(diceerrors.ErrNotImplemented("GETEX"))
}

// Apply similar changes to all other stub functions

Also applies to: 158-160, 162-164, 166-168, 170-172, 174-176, 178-180, 182-184, 186-188, 190-192, 194-196, 198-200, 202-204, 206-208, 210-212

🧰 Tools
🪛 golangci-lint (1.64.8)

154-154: func evalSET is unused

(unused)

internal/eval/main_test.go (1)

14-14: Updated NewStore call with the new signature.

Good job updating the NewStore function call to include the new shardID parameter. Consider using a named constant instead of a magic number 0 to improve code readability.

-	store := dstore.NewStore(nil, nil, 0)
+	const testShardID = 0 // Default shard ID for tests
+	store := dstore.NewStore(nil, nil, testShardID)
internal/store/expire_test.go (1)

13-13: Updated NewStore call to match new function signature.

Good job updating the NewStore function call to include the required shardID parameter. This fixes compatibility with the updated function signature in internal/store/store.go.

-	store := NewStore(nil, nil, 0)
+	const testShardID = 0 // Default shard ID for testing
+	store := NewStore(nil, nil, testShardID)
internal/eval/eval_test.go (1)

8983-8983: Consider testing with different shardID values.

While setting shardID to 0 for all test cases is appropriate for basic unit tests, it might be worth adding a few test cases that use different shardID values to ensure functionality works correctly across different shards.

tests0/main_test.go (2)

22-28: Consider handling connection failure gracefully

While panicking works for tests, consider adding a more graceful fallback mechanism or retry logic for more robust testing, especially in CI environments where network conditions might vary.

func getLocalConnection() *dicedb.Client {
	client, err := dicedb.NewClient("localhost", config.Config.Port)
	if err != nil {
-		panic(err)
+		// Log the error first
+		log.Printf("Failed to connect to DiceDB: %v", err)
+		// Then panic with a more descriptive message
+		panic(fmt.Sprintf("Failed to establish connection to DiceDB at localhost:%d: %v", 
+			config.Config.Port, err))
	}
	return client
}

30-52: Add support for additional response types

The assertEqual function handles many common types but doesn't handle all possible response types from DiceDB. Consider extending it to support additional types such as maps, floats, or other custom types that might be returned.

func assertEqual(t *testing.T, expected interface{}, actual *wire.Response) bool {
	var areEqual bool
	switch v := expected.(type) {
	case string:
		areEqual = v == actual.GetVStr()
	case int64:
		areEqual = v == actual.GetVInt()
	case int:
		areEqual = int64(v) == actual.GetVInt()
	case nil:
		areEqual = actual.GetVNil()
	case error:
		areEqual = v.Error() == actual.Err
	case []*structpb.Value:
		if actual.VList != nil {
			areEqual = reflect.DeepEqual(v, actual.GetVList())
		}
+	case float64:
+		areEqual = v == actual.GetVFloat()
+	case map[string]interface{}:
+		if actual.GetVMap() != nil {
+			areEqual = reflect.DeepEqual(v, actual.GetVMap())
+		}
+	case bool:
+		areEqual = v == actual.GetVBool()
	}
	if !areEqual {
-		t.Errorf("expected %v, got %v", expected, actual)
+		t.Errorf("expected %v (%T), got %v (%T)", expected, expected, actual, actual)
	}
	return areEqual
}
internal/store/batchevictionlru_test.go (1)

107-108: Remove misleading comment about eviction rate

The comment "0% eviction rate" is now misleading since the eviction rate parameter has been moved from NewPrimitiveEvictionStrategy to NewStore, and is set to 0 in the NewStore call.

-	eviction := NewPrimitiveEvictionStrategy(10) // 0% eviction rate
+	eviction := NewPrimitiveEvictionStrategy(10)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cf52728 and 2fd8fdd.

📒 Files selected for processing (9)
  • internal/eval/bloom_test.go (5 hunks)
  • internal/eval/countminsketch_test.go (1 hunks)
  • internal/eval/eval.go (1 hunks)
  • internal/eval/eval_test.go (21 hunks)
  • internal/eval/hmap_test.go (3 hunks)
  • internal/eval/main_test.go (1 hunks)
  • internal/store/batchevictionlru_test.go (7 hunks)
  • internal/store/expire_test.go (1 hunks)
  • tests0/main_test.go (1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
internal/store/expire_test.go (1)
internal/store/store.go (1)
  • NewStore (59-72)
internal/eval/main_test.go (1)
internal/store/store.go (1)
  • NewStore (59-72)
internal/eval/countminsketch_test.go (1)
internal/store/store.go (1)
  • NewStore (59-72)
internal/store/batchevictionlru_test.go (2)
internal/store/batchevictionlru.go (1)
  • NewPrimitiveEvictionStrategy (53-57)
internal/store/store.go (1)
  • NewStore (59-72)
internal/eval/bloom_test.go (1)
internal/store/store.go (1)
  • NewStore (59-72)
internal/eval/eval_test.go (1)
internal/store/store.go (1)
  • NewStore (59-72)
🪛 golangci-lint (1.64.8)
internal/eval/eval.go

146-146: func evalPING is unused

(unused)


150-150: func evalECHO is unused

(unused)


154-154: func evalSET is unused

(unused)


158-158: func evalGETEX is unused

(unused)


162-162: func evalGETDEL is unused

(unused)


166-166: func evalGET is unused

(unused)


170-170: func evalEXPIRE is unused

(unused)


174-174: func evalEXPIRETIME is unused

(unused)


178-178: func evalEXPIREAT is unused

(unused)


182-182: func evalTTL is unused

(unused)

🔇 Additional comments (18)
internal/eval/countminsketch_test.go (1)

14-14:

✅ Verification successful

Updated NewStore call to match new function signature.

The update to include the new shardID parameter in the NewStore function call is correct and necessary to match the updated function signature.

Let's verify if the updated NewStore signature has been consistently applied across all test files in the codebase:


🏁 Script executed:

#!/bin/bash
# Search for any remaining instances of the old NewStore signature
rg -A 1 "NewStore\(.*\)" --glob "*.go" | grep -v ", 0)" | grep -A 1 "NewStore"

Length of output: 398


Consistent Update of NewStore Signature Verified

  • The updated call in internal/shardthread/main.go now correctly passes the required three arguments (nil, evictionStrategy, id) to match the new function signature.
  • The test file internal/eval/countminsketch_test.go uses dummy values (nil, nil, 0), which is acceptable for testing purposes.
  • No outdated calls to NewStore were found elsewhere in the codebase.
internal/eval/hmap_test.go (6)

47-49: Variable cleanup improves code quality

The unused variable has been properly replaced with _ in the error test case, as we only need to verify the error condition and the numSet value, not the actual returned HashMap.


64-66: Properly ignored unused return value

Good cleanup of the test case by discarding the unused return value with _. This follows Go best practices for handling unused values and eliminates potential compiler warnings.


69-71: Improved test case by eliminating unused variable

The removal of the unused variable assignment improves code quality while maintaining the intended error testing functionality.


119-121: Eliminated unused return value

The test case now properly uses _ to discard the unused return value while maintaining the error checking functionality.


124-127: Improved test case with proper value handling

The test now correctly discards the unused return value while still validating the overflow error condition.


129-131: Properly refactored negative overflow test

The negative overflow test has been improved by eliminating the unused variable assignment, following Go best practices.

internal/eval/eval_test.go (6)

54-54: Updated dstore.NewStore call to match the new function signature.

This change correctly adds the new shardID parameter (with value 0) to the dstore.NewStore constructor call, matching the updated function signature in store.go.


1492-1492: Correctly updated NewStore call in BenchmarkEvalJSONOBJLEN.

This change properly adds the required shardID parameter to match the updated constructor signature.


4642-4642: Updated NewStore call in BenchmarkEvalHKEYS to include shardID parameter.

The constructor call has been correctly updated to match the new signature requirements.


4999-4999: Fixed NewStore call in BenchmarkEvalHLEN.

The updated constructor call now correctly includes the shardID parameter as required by the updated signature.


5655-5655: Updated NewStore call in BenchmarkEvalPFCOUNT.

The call was correctly modified to use the new signature with the shardID parameter. Note that this instance dereferences the pointer after creation.


8473-8474: Fixed constructor calls in all remaining benchmark functions.

The updated NewStore calls now correctly include the shardID parameter to comply with the modified signature requirements in store.go.

tests0/main_test.go (1)

15-20: Well-structured TestCase type for command testing

The TestCase struct is well-designed with fields for test name, commands to execute, expected results, and optional delays. This provides a clean foundation for structured testing of DiceDB commands.

internal/store/batchevictionlru_test.go (1)

17-18:

✅ Verification successful

Updated signature for store initialization

The NewPrimitiveEvictionStrategy function now takes a single parameter (no longer requires eviction rate), and NewStore now requires a third parameter for the shard ID. These changes align with the updated signatures in the implementation files.


🏁 Script executed:

#!/bin/bash
# Verify the signatures of the affected functions
echo "Checking NewPrimitiveEvictionStrategy signature:"
rg -n "func NewPrimitiveEvictionStrategy" internal/store/batchevictionlru.go

echo "Checking NewStore signature:"
rg -n "func NewStore" internal/store/store.go

Length of output: 607


Store Initialization Signatures Verified

  • Confirmed that NewPrimitiveEvictionStrategy (in internal/store/batchevictionlru.go at line 53) now accepts only one parameter.
  • Confirmed that NewStore (in internal/store/store.go at line 59) correctly requires three parameters, including the new shard ID.

The initialization code in internal/store/batchevictionlru_test.go using:

eviction := NewPrimitiveEvictionStrategy(5)
s := NewStore(nil, eviction, 0)

is now fully in line with the updated implementation.

internal/eval/bloom_test.go (3)

18-18: Updated store initialization with new parameter

The dstore.NewStore function now takes a third parameter (shard ID), which aligns with the updated function signature. This ensures compatibility with the new store implementation.


99-99: Updated store initialization in test function

The dstore.NewStore function call has been updated with the third parameter for shard ID, ensuring consistency with the updated function signature.


29-29:

✅ Verification successful

Standardized error message format

Error message formats have been normalized by removing the "ERR " prefix and standardizing command case. This improves consistency in error handling throughout the codebase.

Also applies to: 40-40, 46-46, 73-73


🏁 Script executed:

#!/bin/bash
# Verify whether the error messaging format is consistent across other test files
echo "Checking for error message formats in bloom_test.go:"
rg -n "wrong number of arguments for" internal/eval/bloom_test.go

echo "Checking for error message formats in other test files:"
rg -n "wrong number of arguments for" internal/eval/*.go | grep -v bloom_test.go

Length of output: 10814


Standardized Error Message Format Verified

The updated error messages in internal/eval/bloom_test.go now correctly remove the "ERR " prefix and use the standardized command case as expected. Verification shows that:

  • Lines 29 & 40: Use "wrong number of arguments for 'bf.reserve' command" (no ERR prefix).
  • Line 46: Uses "wrong number of arguments for 'BF.ADD' command".
  • Line 73: Uses "wrong number of arguments for 'BF.EXISTS' command".

These modifications align with the normalization approach described.

Comment on lines +54 to +72
func runTestcases(t *testing.T, client *dicedb.Client, testCases []TestCase) {
client.Fire(&wire.Command{
Cmd: "FLUSHDB",
})
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
for i, cmd := range tc.commands {
if len(tc.delay) > i && tc.delay[i] > 0 {
time.Sleep(tc.delay[i])
}
result := client.Fire(&wire.Command{
Cmd: strings.Split(cmd, " ")[0],
Args: strings.Split(cmd, " ")[1:],
})
assertEqual(t, tc.expected[i], result)
}
})
}
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add test case validation and improve error handling

The runTestcases function doesn't validate that the number of commands matches the number of expected results. Also, consider improving the error messages to provide better context when a test fails.

func runTestcases(t *testing.T, client *dicedb.Client, testCases []TestCase) {
	client.Fire(&wire.Command{
		Cmd: "FLUSHDB",
	})
	for _, tc := range testCases {
+		// Validate test case
+		if len(tc.commands) != len(tc.expected) {
+			t.Fatalf("Test case '%s' has mismatched commands (%d) and expected results (%d)",
+				tc.name, len(tc.commands), len(tc.expected))
+		}
+
		t.Run(tc.name, func(t *testing.T) {
			for i, cmd := range tc.commands {
				if len(tc.delay) > i && tc.delay[i] > 0 {
					time.Sleep(tc.delay[i])
				}
+				cmdParts := strings.Split(cmd, " ")
+				cmdName := cmdParts[0]
+				cmdArgs := cmdParts[1:]
				result := client.Fire(&wire.Command{
-					Cmd:  strings.Split(cmd, " ")[0],
-					Args: strings.Split(cmd, " ")[1:],
+					Cmd:  cmdName,
+					Args: cmdArgs,
				})
-				assertEqual(t, tc.expected[i], result)
+				if !assertEqual(t, tc.expected[i], result) {
+					t.Logf("Command failed: %s", cmd)
+				}
			}
		})
	}
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func runTestcases(t *testing.T, client *dicedb.Client, testCases []TestCase) {
client.Fire(&wire.Command{
Cmd: "FLUSHDB",
})
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
for i, cmd := range tc.commands {
if len(tc.delay) > i && tc.delay[i] > 0 {
time.Sleep(tc.delay[i])
}
result := client.Fire(&wire.Command{
Cmd: strings.Split(cmd, " ")[0],
Args: strings.Split(cmd, " ")[1:],
})
assertEqual(t, tc.expected[i], result)
}
})
}
}
func runTestcases(t *testing.T, client *dicedb.Client, testCases []TestCase) {
client.Fire(&wire.Command{
Cmd: "FLUSHDB",
})
for _, tc := range testCases {
// Validate test case
if len(tc.commands) != len(tc.expected) {
t.Fatalf("Test case '%s' has mismatched commands (%d) and expected results (%d)",
tc.name, len(tc.commands), len(tc.expected))
}
t.Run(tc.name, func(t *testing.T) {
for i, cmd := range tc.commands {
if len(tc.delay) > i && tc.delay[i] > 0 {
time.Sleep(tc.delay[i])
}
cmdParts := strings.Split(cmd, " ")
cmdName := cmdParts[0]
cmdArgs := cmdParts[1:]
result := client.Fire(&wire.Command{
Cmd: cmdName,
Args: cmdArgs,
})
if !assertEqual(t, tc.expected[i], result) {
t.Logf("Command failed: %s", cmd)
}
}
})
}
}

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
internal/eval/eval.go (1)

154-159: ⚠️ Potential issue

Fix the ECHO command logic.

There appears to be a logical error in the evalECHO implementation. It returns an error if there's exactly one argument, but the ECHO command should actually return an error if there is NOT exactly one argument.

func evalECHO(args []string, store *dstore.Store) []byte {
-	if len(args) == 1 {
+	if len(args) != 1 {
		return diceerrors.NewErrArity("ECHO")
	}
	return Encode(args[0], false)
}
🧰 Tools
🪛 golangci-lint (1.64.8)

154-154: func evalECHO is unused

(unused)


154-154: evalECHO - store is unused

(unparam)

🧹 Nitpick comments (1)
internal/eval/eval.go (1)

161-163: Add TODO comments and documentation for stub functions.

All these new functions are returning nil and lack implementation details and documentation. While adding these stubs likely addresses compiler warnings, they should include TODO comments to indicate future implementation plans and proper documentation explaining their purpose.

Consider adding documentation and TODO comments to each function. For example:

func evalSET(args []string, store *dstore.Store) *EvalResponse {
+	// TODO: Implement SET command handler
+	// SET key value [EX seconds|PX milliseconds|EXAT timestamp|PXAT milliseconds-timestamp|KEEPTTL] [NX|XX]
	return nil
}

Also note the return type inconsistency: evalPING and evalECHO return []byte while the remaining functions return *EvalResponse. Consider standardizing the return types.

Also applies to: 165-167, 169-171, 173-175, 177-179, 181-183, 185-187, 189-191, 193-195, 197-199, 201-203, 205-207, 209-211, 213-215, 217-219

🧰 Tools
🪛 golangci-lint (1.64.8)

161-161: func evalSET is unused

(unused)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2fd8fdd and 858d8b0.

📒 Files selected for processing (1)
  • internal/eval/eval.go (1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
internal/eval/eval.go

146-146: func evalPING is unused

(unused)


146-146: evalPING - store is unused

(unparam)


154-154: func evalECHO is unused

(unused)


154-154: evalECHO - store is unused

(unparam)


161-161: func evalSET is unused

(unused)


165-165: func evalGETEX is unused

(unused)


169-169: func evalGETDEL is unused

(unused)


173-173: func evalGET is unused

(unused)


177-177: func evalEXPIRE is unused

(unused)


181-181: func evalEXPIRETIME is unused

(unused)


185-185: func evalEXPIREAT is unused

(unused)


189-189: func evalTTL is unused

(unused)

🔇 Additional comments (1)
internal/eval/eval.go (1)

146-152:

✅ Verification successful

Good implementation of the PING command.

The evalPING function is now correctly implemented as suggested in a previous review comment, following the Redis protocol standard. However, static analysis indicates this function is currently unused in the codebase and the store parameter is not used in the implementation.


🏁 Script executed:

#!/bin/bash
# Check if evalPING is connected to any command handler
echo "Checking if evalPING is connected to any command handler"
rg -A 2 -B 2 "evalPING" --glob "!internal/eval/eval.go"

Length of output: 1075


Verified: evalPING is Appropriately Integrated

Good implementation of the PING command. Verification confirms that the evalPING function is correctly wired in the codebase – it’s referenced in the command handler (internal/cmd/cmd_ping.go) and exercised in the evaluation tests (internal/eval/eval_test.go). The unused store parameter appears to be kept for interface consistency; if this is intentional, no further changes are required.

🧰 Tools
🪛 golangci-lint (1.64.8)

146-146: func evalPING is unused

(unused)


146-146: evalPING - store is unused

(unparam)

Comment on lines +146 to +219
func evalPING(args []string, store *dstore.Store) []byte {
// PING command returns PONG if no argument is provided, otherwise it returns the argument
if len(args) == 0 {
return Encode("PONG", false)
}
return Encode(args[0], false)
}

func evalECHO(args []string, store *dstore.Store) []byte {
if len(args) == 1 {
return diceerrors.NewErrArity("ECHO")
}
return Encode(args[0], false)
}

func evalSET(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalGETEX(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalGETDEL(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalGET(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalEXPIRE(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalEXPIRETIME(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalEXPIREAT(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalTTL(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalDEL(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalTYPE(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalSETEX(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalINCR(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalINCRBY(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalDECR(args []string, store *dstore.Store) *EvalResponse {
return nil
}

func evalDECRBY(args []string, store *dstore.Store) *EvalResponse {
return nil
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Create a registration mechanism for command handlers.

All the eval functions are currently flagged as unused by static analysis. A common pattern for command handlers is to register them in a map or command table, so they can be looked up dynamically.

Consider implementing a command registration mechanism to connect these handlers to their respective commands:

// commandHandlers maps command names to their handler functions
var commandHandlers = map[string]func([]string, *dstore.Store) interface{}{
    "PING": evalPING,
    "ECHO": evalECHO,
    // Add other commands as they are implemented
}

// GetCommandHandler returns the handler function for a given command
func GetCommandHandler(command string) (func([]string, *dstore.Store) interface{}, bool) {
    handler, exists := commandHandlers[command]
    return handler, exists
}

This would solve the "unused function" warnings while providing a structured way to look up command handlers.

🧰 Tools
🪛 golangci-lint (1.64.8)

146-146: func evalPING is unused

(unused)


146-146: evalPING - store is unused

(unparam)


154-154: func evalECHO is unused

(unused)


154-154: evalECHO - store is unused

(unparam)


161-161: func evalSET is unused

(unused)


165-165: func evalGETEX is unused

(unused)


169-169: func evalGETDEL is unused

(unused)


173-173: func evalGET is unused

(unused)


177-177: func evalEXPIRE is unused

(unused)


181-181: func evalEXPIRETIME is unused

(unused)


185-185: func evalEXPIREAT is unused

(unused)


189-189: func evalTTL is unused

(unused)

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.

3 participants