This is an archive of the discontinued Mercurial Phabricator instance.

wireprotov2: declare command arguments richly
ClosedPublic

Authored by indygreg on Sep 17 2018, 5:23 PM.

Details

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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 17 2018, 5:23 PM
durin42 added inline comments.
mercurial/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?

indygreg added inline comments.Sep 18 2018, 5:50 PM
mercurial/wireprotov2server.py
543–546

We could do that, sure. Would it be OK if I made that change in D4617 instead?

indygreg updated this revision to Diff 11243.Sep 20 2018, 5:14 PM
durin42 added inline comments.Sep 24 2018, 9:24 AM
mercurial/wireprotov2server.py
547

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.

This revision was automatically updated to reflect the committed changes.
indygreg added inline comments.Sep 24 2018, 12:17 PM
mercurial/wireprotov2server.py
601–604

Does this added check not address your review feedback, @durin42?

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?

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.