Conversation
promag
left a comment
promag
left a comment
There was a problem hiding this comment.
Concept ACK. Should mention in developer notes that named params is preferable for new code.
|
Well, my hope is that after this is merged, we can migrate existing code to use it and drop |
|
The |
|
100 args are only needed to throw an internal bug report. You can replace that with a check that checks if the first argument is the string "throw_internal_bug_report" (or anything else you can come up with) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
luke-jr
force-pushed
the
internal_named_params
branch
2 times, most recently
from
8186d29 to
e52fa66
Compare
|
Concept ACK |
|
Alternative fix for |
…cts instead of entire JSONRPCRequests
With internal named parameters, all parameters MUST have names
luke-jr
force-pushed
the
internal_named_params
branch
2 times, most recently
from
2735696 to
f47bd98
Compare
|
Rebased. Thanks to changes in master, neither hack is needed anymore. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Doesn't seem to be much point in rebasing this. It'd be nice to have, but without any review interest, just wasting time. |
|
Concept ACK |
yanmaani
left a comment
yanmaani
left a comment
There was a problem hiding this comment.
Concept ACK. This would make a lot of code much cleaner!
| static inline JSONRPCRequest transformArguments(const JSONRPCRequest& in, const std::vector<std::string>& argNames) | ||
| { | ||
| auto out = in; | ||
| if (in.params.isObject()) { |
There was a problem hiding this comment.
Nit: wouldn't it be slightly cleaner to do something like:
bool isPositional = in.params.isObject();
out.m_used_positional_args = isPositional;
if (isPositional) { ... }
Same loc count, but reads slightly cleaner IMHO.
| out.params = transformNamedArguments(in.params, argNames); | ||
| } else { | ||
| out.m_used_positional_args = true; | ||
| if (in.params.size() > argNames.size()) { |
There was a problem hiding this comment.
Is there some reason to do this after setting out.m_used_positional_args? Otherwise it seems either cleaner to put it first, or to put it last and take out the return out;.
8 participants
Reworked #11660 to add a new
m_paramstoJSONRPCRequestin parallel to the legacyparamsto ease the transition.Dropped other unnecessary improvements and "options" handling for now to keep this PR smaller, those can go in later in other PRs.