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

Reviews

See the guideline for information on the review process.

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

Conflicts

No conflicts as of last run.

I get that this is confusing, but this change isn't correct.

The "data" field itself is required when the object is present (as in [{"address": amount, ..., {}] would not be valid). What's optional is the object (the { ... } around it), which is at a higher level.

Maybe it's worth elaborating in the prose text above?

I get that this is confusing, but this change isn't correct.

The "data" field itself is required when the object is present (as in [{"address": amount, ..., {}] would not be valid). What's optional is the object (the { ... } around it), which is at a higher level.

Maybe it's worth elaborating in the prose text above?

Understood you're saying the change isn't correct. How would someone properly indicate the "data" is optional if not specifying it as optional? The parent output is required and some of it's children elements are optional. From my perspective what I would expect to see is a required "outputs" and within outputs there being required and optional fields.

For instance: "address": amount, (numeric or string, required makes sense as address and amount are necessary.
But "data": "hex", (string, required) seems like a bug because this is not required and can be left out. If you specify "data" then of course there will be a required value for it, but this is implied with how json works.

Full text of for outputs as reference:

2. outputs                            (json array, required) The outputs (key-value pairs), where none of the keys are duplicated.
                                      That is, each address can only appear once and there can only be one 'data' object.
                                      For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also
                                      accepted as second parameter.
     [
       {                              (json object)
         "address": amount,           (numeric or string, required) A key-value pair. The key (string) is the bitcoin address,
                                      the value (float or string) is the amount in BTC
         ...
       },
       {                              (json object)
         "data": "hex",               (string, required) A key-value pair. The key must be "data", the value is hex-encoded data
       },
       ...
     ]

The "data": ... part cannot be left out. It's the {"data": ...} that can be left out.

The "data": ... part cannot be left out. It's the {"data": ...} that can be left out.

Let me rephrase my question:
How would someone know {"data" ...} is optional if not specifying it as optional? If every field is optional within an obj maybe it's implied the obj itself is optional?

Does there need to be a new idea of specifying whether the (json object) itself is optional?

Edit: Is this what "prose text" means?

@sipa, edited the commit. Something like this look good?

ACK 30acffa

@Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.

@Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.

I think you have correctly identified that this text could be improved and we should move forward with the PR.

@miketwenty1 Needs rebase. Are you still working on this?

@jonatack thanks for the bump. Open for feedback. I took @murchandamus suggestions where it seemed appropriate.

I think the CI failure is unrelated to your change (see #28027).

ACK 27b168b