D4615: wireprotov2: declare command arguments richly
indygreg added a comment. In https://phab.mercurial-scm.org/D4615#71677, @durin42 wrote: > No, actually - I was proposing *removing* `required` entirely, and *only* keying off the existence of a default. What you reduces us to only two valid states: required-with-no-default and not-required-with-default. I think we could make the specification of a default the thing that makes the argument optional. WDYT? As far as the internal code API, I think removing `required` is acceptable. I'll code up a patch to do that. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4615 To: indygreg, #hg-reviewers Cc: indygreg, durin42, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D4615: wireprotov2: declare command arguments richly
durin42 added a subscriber: indygreg.
durin42 added a comment.
> indygreg added inline comments.
>
> INLINE COMMENTS
>
>> wireprotov2server.py:602-605
>> +if 'default' in meta and meta.get('required'):
>> +raise error.ProgrammingError('%s argument for command %s is
marked '
>> + 'as required but has a default
value' %
>> + (arg, name))
>
> Does this added check not address your review feedback, @durin42?
No, actually - I was proposing *removing* `required` entirely, and *only*
keying off the existence of a default. What you reduces us to only two valid
states: required-with-no-default and not-required-with-default. I think we
could make the specification of a default the thing that makes the argument
optional. WDYT?
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D4615
To: indygreg, #hg-reviewers
Cc: indygreg, durin42, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D4615: wireprotov2: declare command arguments richly
indygreg added inline comments.
INLINE COMMENTS
> wireprotov2server.py:602-605
> +if 'default' in meta and meta.get('required'):
> +raise error.ProgrammingError('%s argument for command %s is
> marked '
> + 'as required but has a default
> value' %
> + (arg, name))
Does this added check not address your review feedback, @durin42?
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D4615
To: indygreg, #hg-reviewers
Cc: durin42, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D4615: wireprotov2: declare command arguments richly
This revision was automatically updated to reflect the committed changes. Closed by commit rHG0b61d21f05cc: wireprotov2: declare command arguments richly (authored by indygreg, committed by ). REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D4615?vs=11243=11302 REVISION DETAIL https://phab.mercurial-scm.org/D4615 AFFECTED FILES mercurial/wireprotov2server.py tests/test-http-protocol.t tests/test-wireproto-command-capabilities.t tests/test-wireproto-command-filedata.t tests/test-wireproto-command-manifestdata.t CHANGE DETAILS diff --git a/tests/test-wireproto-command-manifestdata.t b/tests/test-wireproto-command-manifestdata.t --- a/tests/test-wireproto-command-manifestdata.t +++ b/tests/test-wireproto-command-manifestdata.t @@ -65,14 +65,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 45\r\n - s> =\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror + s> 4e\r\n + s> F\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, treeFstatusEerror s> \r\n - received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: nodes argument must be defined! + abort: missing required arguments: nodes, tree! [255] $ sendhttpv2peer << EOF @@ -97,14 +97,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 44\r\n - s> <\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1dtree argument must be definedFstatusEerror + s> 47\r\n + s> ?\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX missing required arguments: treeFstatusEerror s> \r\n - received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: tree argument must be defined! + abort: missing required arguments: tree! [255] Unknown node is an error diff --git a/tests/test-wireproto-command-filedata.t b/tests/test-wireproto-command-filedata.t --- a/tests/test-wireproto-command-filedata.t +++ b/tests/test-wireproto-command-filedata.t @@ -69,14 +69,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 45\r\n - s> =\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror + s> 4e\r\n + s> F\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, pathFstatusEerror s> \r\n - received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: nodes argument must be defined! + abort: missing required arguments: nodes, path! [255] $ sendhttpv2peer << EOF @@ -101,14 +101,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 44\r\n - s> <\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1dpath argument must be definedFstatusEerror + s> 47\r\n + s> ?\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX missing required arguments: pathFstatusEerror s> \r\n - received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: path argument must be defined! + abort: missing required arguments: path! [255] Unknown node is an error diff --git a/tests/test-wireproto-command-capabilities.t b/tests/test-wireproto-command-capabilities.t --- a/tests/test-wireproto-command-capabilities.t +++ b/tests/test-wireproto-command-capabilities.t @@ -212,7 +212,7 @@ s> Content-Type: application/mercurial-cbor\r\n s> Content-Length: *\r\n (glob) s> \r\n - s>
D4615: wireprotov2: declare command arguments richly
durin42 added inline comments. INLINE COMMENTS > wireprotov2server.py:547 > + > + ``required`` > + Bool indicating whether the argument is required. https://phab.mercurial-scm.org/D4617 no longer exists, so I'm taking this change as-is, but expect a follow-up (at least conversation) about the default-vs-required business. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4615 To: indygreg, #hg-reviewers Cc: durin42, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D4615: wireprotov2: declare command arguments richly
indygreg updated this revision to Diff 11243. REPOSITORY rHG Mercurial CHANGES SINCE LAST UPDATE https://phab.mercurial-scm.org/D4615?vs=4=11243 REVISION DETAIL https://phab.mercurial-scm.org/D4615 AFFECTED FILES mercurial/wireprotov2server.py tests/test-http-protocol.t tests/test-wireproto-command-capabilities.t tests/test-wireproto-command-filedata.t tests/test-wireproto-command-manifestdata.t CHANGE DETAILS diff --git a/tests/test-wireproto-command-manifestdata.t b/tests/test-wireproto-command-manifestdata.t --- a/tests/test-wireproto-command-manifestdata.t +++ b/tests/test-wireproto-command-manifestdata.t @@ -65,14 +65,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 45\r\n - s> =\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror + s> 4e\r\n + s> F\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, treeFstatusEerror s> \r\n - received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: nodes argument must be defined! + abort: missing required arguments: nodes, tree! [255] $ sendhttpv2peer << EOF @@ -97,14 +97,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 44\r\n - s> <\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1dtree argument must be definedFstatusEerror + s> 47\r\n + s> ?\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX missing required arguments: treeFstatusEerror s> \r\n - received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: tree argument must be defined! + abort: missing required arguments: tree! [255] Unknown node is an error diff --git a/tests/test-wireproto-command-filedata.t b/tests/test-wireproto-command-filedata.t --- a/tests/test-wireproto-command-filedata.t +++ b/tests/test-wireproto-command-filedata.t @@ -69,14 +69,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 45\r\n - s> =\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror + s> 4e\r\n + s> F\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, pathFstatusEerror s> \r\n - received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: nodes argument must be defined! + abort: missing required arguments: nodes, path! [255] $ sendhttpv2peer << EOF @@ -101,14 +101,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 44\r\n - s> <\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1dpath argument must be definedFstatusEerror + s> 47\r\n + s> ?\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX missing required arguments: pathFstatusEerror s> \r\n - received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: path argument must be defined! + abort: missing required arguments: path! [255] Unknown node is an error diff --git a/tests/test-wireproto-command-capabilities.t b/tests/test-wireproto-command-capabilities.t --- a/tests/test-wireproto-command-capabilities.t +++ b/tests/test-wireproto-command-capabilities.t @@ -212,7 +212,7 @@ s> Content-Type: application/mercurial-cbor\r\n s> Content-Length: *\r\n (glob) s> \r\n - s>
D4615: wireprotov2: declare command arguments richly
indygreg added inline comments. INLINE COMMENTS > durin42 wrote in wireprotov2server.py:543-546 > Shouldn't there be a default XOR required == true? That is, could we get away > with `default=None` and then deriving requiredness from default-availability? We could do that, sure. Would it be OK if I made that change in https://phab.mercurial-scm.org/D4617 instead? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4615 To: indygreg, #hg-reviewers Cc: durin42, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D4615: wireprotov2: declare command arguments richly
durin42 added inline comments. INLINE COMMENTS > wireprotov2server.py:543-546 > + ``default`` > + A callable returning the default value for this argument. > + > + ``required`` Shouldn't there be a default XOR required == true? That is, could we get away with `default=None` and then deriving requiredness from default-availability? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4615 To: indygreg, #hg-reviewers Cc: durin42, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D4615: wireprotov2: declare command arguments richly
indygreg created this revision. Herald added a subscriber: mercurial-devel. Herald added a reviewer: hg-reviewers. REVISION SUMMARY Previously, we declared command arguments with an example of their value. After this commit, we declare command arguments as a dict of metadata. This allows us to define the value type, whether the argument is required, and provide default values. This in turn allows us to have nice things, such as less boilerplate code in individual commands for validating input and assigning default values. It should also make command behavior more consistent as a result. Test output changed slightly because I realized that the "fields" argument wasn't being consistently defined as a set. Oops! Other test output changed because of slight differences in code performing type validation. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D4615 AFFECTED FILES mercurial/wireprotov2server.py tests/test-http-protocol.t tests/test-wireproto-command-capabilities.t tests/test-wireproto-command-filedata.t tests/test-wireproto-command-manifestdata.t CHANGE DETAILS diff --git a/tests/test-wireproto-command-manifestdata.t b/tests/test-wireproto-command-manifestdata.t --- a/tests/test-wireproto-command-manifestdata.t +++ b/tests/test-wireproto-command-manifestdata.t @@ -65,14 +65,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 45\r\n - s> =\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror + s> 4e\r\n + s> F\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, treeFstatusEerror s> \r\n - received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: nodes argument must be defined! + abort: missing required arguments: nodes, tree! [255] $ sendhttpv2peer << EOF @@ -97,14 +97,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 44\r\n - s> <\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1dtree argument must be definedFstatusEerror + s> 47\r\n + s> ?\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX missing required arguments: treeFstatusEerror s> \r\n - received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: tree argument must be defined! + abort: missing required arguments: tree! [255] Unknown node is an error diff --git a/tests/test-wireproto-command-filedata.t b/tests/test-wireproto-command-filedata.t --- a/tests/test-wireproto-command-filedata.t +++ b/tests/test-wireproto-command-filedata.t @@ -69,14 +69,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 45\r\n - s> =\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1enodes argument must be definedFstatusEerror + s> 4e\r\n + s> F\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX\'missing required arguments: nodes, pathFstatusEerror s> \r\n - received frame(size=61; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=70; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: nodes argument must be defined! + abort: missing required arguments: nodes, path! [255] $ sendhttpv2peer << EOF @@ -101,14 +101,14 @@ s> Content-Type: application/mercurial-exp-framing-0005\r\n s> Transfer-Encoding: chunked\r\n s> \r\n - s> 44\r\n - s> <\x00\x00\x01\x00\x02\x012 - s> \xa2Eerror\xa1GmessageX\x1dpath argument must be definedFstatusEerror + s> 47\r\n + s> ?\x00\x00\x01\x00\x02\x012 + s> \xa2Eerror\xa1GmessageX missing required arguments: pathFstatusEerror s> \r\n - received frame(size=60; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) + received frame(size=63; request=1; stream=2; streamflags=stream-begin; type=command-response; flags=eos) s> 0\r\n s> \r\n - abort: path argument must be defined! + abort: missing required arguments: path! [255] Unknown node is an error diff --git a/tests/test-wireproto-command-capabilities.t b/tests/test-wireproto-command-capabilities.t --- a/tests/test-wireproto-command-capabilities.t +++ b/tests/test-wireproto-command-capabilities.t @@ -212,7 +212,7 @@
