Implement help <method-name> by liamgilligan · Pull Request #300 · ConsensusJ/consensusj

liamgilligan · 2026-01-23T01:50:02Z

PR for #260

Currently filler text is being used, tests have been written/modified to account for new help behavior.

msgilligan

I know this is a draft, but I have a couple suggestions

consensusj-jrpc-echod/src/main/java/org/consensusj/jsonrpc/daemon/EchoJsonRpcService.java

consensusj-jrpc-echod/src/test/java/org/consensusj/jsonrpc/daemon/ApplicationTest.java

msgilligan

Additional comment.

consensusj-jrpc-echod/src/main/java/org/consensusj/jsonrpc/daemon/EchoJsonRpcService.java

msgilligan · 2026-01-24T23:00:35Z

@liamgilligan See PR #303

msgilligan · 2026-01-26T20:01:54Z

Please rebase now that PR #303 is merged. Also see WIP PR #309.

We should add a JsonRpcHelp (or similar name) class a new .help subpackage of o.c.jsonrpc. Because this module is currently JDK 11, we can't use record, but we can create a record-like class and convert to a record for 0.8.0.

msgilligan

A few requested changes...

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/help/JsonRpcHelp.java

consensusj-jrpc-echod/src/test/java/org/consensusj/jsonrpc/daemon/ApplicationTest.java

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/services/EchoJsonRpcService.java

Added short summary and detailed help text, added and updated tests, and added javadoc and copyright headers when applicable.

msgilligan

Looks good. I have a few requests.

msgilligan · 2026-03-05T03:51:03Z

consensusj-jrpc-echod/src/test/java/org/consensusj/jsonrpc/daemon/ApplicationTest.java

+    @Test
+    void helpForHelpMethod() throws IOException {
+        var expectedResultSubstring  = "Displays detailed help text for the specified method.";
+        try (var client = new DefaultRpcClient(endpoint, "", "")) {


Use @BeforeEach to initialize client for each test. Use @AfterEach to call client.close(). This will make the individual tests more readable by eliminating the call to DefaultRpcClient and the try block.

msgilligan · 2026-03-05T03:52:24Z

consensusj-jrpc-echod/src/test/java/org/consensusj/jsonrpc/daemon/ApplicationTest.java

+        var expectedResultSubstring  = "Displays detailed help text for the specified method.";
+        try (var client = new DefaultRpcClient(endpoint, "", "")) {
+            String result = (String) client.send("help", "help");
+            assertTrue(result.contains(expectedResultSubstring));


For now we don't need to match the entire text of the result, but we should verify that the result contains the method name and the correct argument name (and optional parens) So that's three asserts per test:

contains method name

contains parameter names and optionality info

contains descriptive text

msgilligan · 2026-03-05T04:03:29Z

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/services/EchoJsonRpcService.java

+                            + "Parameters:\n"
+                            + "  None.\n")
+    );
+    private static final String helpString = createHelpString(EchoJsonRpcService::createHelpStringLine);


This member should probably be renamed something like allMethodsHelpString or similar and have a JavaDoc comment explaining what it is.

msgilligan · 2026-03-05T04:04:12Z

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/services/EchoJsonRpcService.java


-    public CompletableFuture<String> help() {
+    /**
+     * Get detailed help information for a given command.


... or all commands.

msgilligan · 2026-03-05T04:04:36Z

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/services/EchoJsonRpcService.java

-    public CompletableFuture<String> help() {
+    /**
+     * Get detailed help information for a given command.
+     * @param method: A string containing the method name


... or null for all commands.

msgilligan · 2026-03-05T04:06:20Z

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/services/EchoJsonRpcService.java

+        if (method == null) {
+            return result(helpString);
+        }
+        if (helpMap.containsKey(method)) {


I would use an else if here, just to be a little more clear that exactly one of the three conditions will be true.

msgilligan · 2026-03-05T04:07:29Z

consensusj-jsonrpc/src/main/java/org/consensusj/jsonrpc/services/EchoJsonRpcService.java

    }
+
+    /**
+     * Create a string containing the name of each method and it's parameters in alphabetical order.


No apostrophe on "it's"

msgilligan requested changes Jan 23, 2026

View reviewed changes

msgilligan reviewed Jan 23, 2026

View reviewed changes

consensusj-jrpc-echod/src/main/java/org/consensusj/jsonrpc/daemon/EchoJsonRpcService.java Outdated Show resolved Hide resolved

liamgilligan force-pushed the walletd-help-implementation branch from 4b0f436 to c6e6377 Compare January 30, 2026 01:24

liamgilligan requested a review from msgilligan January 30, 2026 22:31

liamgilligan force-pushed the walletd-help-implementation branch from c6e6377 to d7729fa Compare February 5, 2026 22:43

msgilligan requested changes Feb 5, 2026

View reviewed changes

liamgilligan force-pushed the walletd-help-implementation branch 4 times, most recently from 3f149c6 to f57f010 Compare February 27, 2026 02:03

liamgilligan marked this pull request as ready for review February 27, 2026 02:05

liamgilligan requested a review from msgilligan February 27, 2026 02:05

liamgilligan mentioned this pull request Mar 3, 2026

jrpc-echod, walletd: implement help <method-name> #260

Open

Implemented help for method

65101e1

Added short summary and detailed help text, added and updated tests, and added javadoc and copyright headers when applicable.

msgilligan force-pushed the walletd-help-implementation branch from f57f010 to 65101e1 Compare March 5, 2026 03:31

msgilligan requested changes Mar 5, 2026

View reviewed changes

msgilligan mentioned this pull request Mar 5, 2026

Add more JSON-RPC integration tests to jrpc-echod #259

Open

Conversation

liamgilligan commented Jan 23, 2026

Uh oh!

msgilligan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msgilligan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

msgilligan commented Jan 24, 2026

Uh oh!

msgilligan commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msgilligan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msgilligan left a comment

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

msgilligan Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

msgilligan commented Jan 26, 2026 •

edited

Loading