scripted-diff: use RPCArg::Optional::OMITTED over OMITTED_NAMED_ARG #26919

1. 
   fanquake commented at 10:37 am on January 19, 2023: member

   Remove deprecated RPCArg::Optional::OMITTED_NAMED_ARG in favour of OMITTED.
2. 
   DrahtBot commented at 10:37 am on January 19, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	kouloumos, aureleoules

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26485 (RPC: Accept options as named-only parameters by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label Refactoring on Jan 19, 2023
4. 
   maflcko commented at 3:37 pm on January 19, 2023: member

   The scripted diff can be:

   0sed -i -e "/Deprecated alias for OMITTED, can be removed/d" src/rpc/util.{h,cpp}
   1sed -i -e "s/OMITTED_NAMED_ARG/OMITTED/g" $(git grep -l "OMITTED_NAMED_ARG" src/)
   

5. fanquake force-pushed on Jan 19, 2023
6. 
   fanquake commented at 3:40 pm on January 19, 2023: member

   The scripted diff can be:

   Sure. Updated.

7. fanquake force-pushed on Jan 19, 2023
8. fanquake requested review from stickies-v on Jan 20, 2023
9. fanquake requested review from kouloumos on Jan 20, 2023
10. in src/rpc/util.h:162 in 92e7a51579 outdated

    164@@ -165,7 +165,6 @@ struct RPCArg {
    165          * When possible, the default value should be specified.
    

    ---

    ---

    

    kouloumos commented at 7:21 pm on January 20, 2023:

    Reading this comment and even going through the changes of how this came to be, I still was a bit confused about the options here as part of the overall logic. The removal of OMITTED_NAMED_ARG definitely makes it easier but may I suggest a different wording for the comments:

    0        /**
    1         * Optional argument for which the default value is omitted from
    2         * help text for one of two reasons:
    3         * - It's a named argument and has a default value of `null`.
    4         * - Its default value is implicitly clear. That is, elements in an
    5         *    array may not exist by default.
    6         * When possible, the default value should be specified.
    

    ---

    

    fanquake commented at 3:07 pm on January 22, 2023:

    Taken your suggestions.
11. 
    kouloumos commented at 8:23 pm on January 20, 2023: contributor

    ACK 92e7a51579712c8108d565a639b206e7555d58e9 I verified that there are no other occurrences of OMITTED_NAMED_ARG. I’ve also verified that there is no diff at the rendered RPC doc after this change by using the RPCdump-patch from #14726#pullrequestreview-175423309.

     0diff --git a/src/init.cpp b/src/init.cpp
     1index 685583bcb..5120d23b5 100644
     2--- a/src/init.cpp
     3+++ b/src/init.cpp
     4@@ -1190,6 +1190,30 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     5     for (const auto& client : node.chain_clients) {
     6         client->registerRpcs();
     7     }
     8+    JSONRPCRequest jreq;
     9+    jreq.mode = JSONRPCRequest::GET_HELP;
    10+    UniValue unused_result;
    11+    for (const auto& command : tableRPC.mapCommands) {
    12+        const CRPCCommand *pcmd = command.second.front();
    13+        try {
    14+            pcmd->actor(jreq, unused_result, true);
    15+        } catch (const std::exception& e) {
    16+            printf("===\n%s\n", e.what());
    17+            continue;
    18+        } catch (const UniValue& e) {
    19+            printf("===\n%s\n", e.write().c_str());
    20+            continue;
    21+        } catch (...) {
    22+            printf("===\n%s\n", pcmd->name.c_str());
    23+            fflush(stdout);
    24+            assert(0);
    25+        }
    26+        printf("===\n%s\n", pcmd->name.c_str());
    27+        fflush(stdout);
    28+        assert(0);
    29+    }
    30+    fflush(stdout);
    31+    abort();
    32 #if ENABLE_ZMQ
    33     RegisterZMQRPCCommands(tableRPC);
    34 #endif
    35diff --git a/src/rpc/server.h b/src/rpc/server.h
    36index 01e855605..130819a56 100644
    37--- a/src/rpc/server.h
    38+++ b/src/rpc/server.h
    39@@ -124,7 +124,7 @@ public:
    40  */
    41 class CRPCTable
    42 {
    43-private:
    44+public:
    45     std::map<std::string, std::vector<const CRPCCommand*>> mapCommands;
    46 public:
    47     CRPCTable();
    

12. scripted-diff: use RPCArg::Optional::OMITTED over OMITTED_NAMED_ARG

    -BEGIN VERIFY SCRIPT-
    sed -i -e "/Deprecated alias for OMITTED, can be removed/d" src/rpc/util.h src/rpc/util.cpp
    sed -i -e "s/OMITTED_NAMED_ARG/OMITTED/g" $(git grep -l "OMITTED_NAMED_ARG" src/)
    -END VERIFY SCRIPT-

    ea8c7daf7a
13. doc: improve doc for RPCArg::Optional::OMITTED 83f70c8e86
14. fanquake force-pushed on Jan 22, 2023
15. 
    kouloumos commented at 8:43 am on January 23, 2023: contributor

    re-ACK 83f70c8e860556426305b1d70fa0a3276784e360
16. aureleoules approved
17. 
    aureleoules commented at 9:11 am on January 23, 2023: member

    ACK 83f70c8e860556426305b1d70fa0a3276784e360
18. maflcko merged this on Jan 23, 2023
19. maflcko closed this on Jan 23, 2023

    ---

20. fanquake deleted the branch on Jan 23, 2023
21. sidhujag referenced this in commit 049aecafe4 on Jan 23, 2023
22. bitcoin locked this on Jan 23, 2024

Contributors
fanquake DrahtBot maflcko kouloumos aureleoules

Review Requested
stickies-v

Labels
Refactoring