Type enforcement for twisted.python.usage

[itamarst]

"I want --port to be an integer."

[therve]

Done in usage-typed-739.

[glyph]

For the record, you don't want --port to be an integer. You want it to be an endpoint :).

[itamarst]

Any reason why types shouldn't be arbitrary callables that take string and either return object, or raise ValueError? Then you could use int and float directly... or function endPointParser if you have one.

[itamarst]

i.e., you'd do ["length", "l", 0, "Length of file to read out", int].

[therve]

That seemed reasonable. Ready to re-review. And then we can have --port to be an endpoint, if you can build an endpoint from a string :).

[exarkun]

A few style points:

• Technically, there is no casting in Python. coerce' or parse' or `convert' or something like that might be better language to use.
• The special method __call__ is somewhat lame. It gives up the documentation/mnemonic value of a function name in exchange for a trivial amount of added convenience. Naming it something descriptive and then grabbing that method from the instance achieves the same affect in a better way.
• Does _CastParameter need to be a nested class? I know there are some nested classes in Twisted already (I even wrote some of them), but they're probably mostly mistakes. Nesting doesn't buy much, and Python doesn't handle it well (eg, since module + '.' + name no longer gives the FQPN of the class)
• Explicitly testing that paramType is callable seems unnecessary, or at least the code is misplaced. Class definition time would be a much better time for this error to occur, but I'm not sure it really needs to occur at all. There's plenty of ways the application author could screw up that will break things. Trying to catch them all is hopeless and just clutters the code. There's also the separate issue concerning the fact that callable() isn't always accurate (various terrible __getattr__ and __getattribute__ implementations can mess it up), and it still says nothing about, eg, the arity of the callable, so it still might break when it's invoked.

The documentation should probably spell out whether default values are passed to the coercion function or not. The current implementation doesn't do this

Thanks for doing the string module, whitespace and other cleanups. I notice a few calls to dict.has_key near some of the changes. If you want, you can change those to use in instead, too.

Something not mentioned before on this ticket is how type requirements are documented (to the user). That might be out of scope for this ticket, but it might also benefit from a slightly different API for this feature, so it might be worth thinking about now. The primary feature I have in mind is avoiding duplicating strings like "Port numbers must be integers between 0 and 65535 (inclusive)", just as this feature will allow the code which enforces that to not be duplicated. Raising a ValueError from the parsing function can probably supply this message in the error case, which might be sufficient, but there might also be value in displaying it in the --help output.

[exarkun]

Oops, didn't finish that thought in the middle.

"The current implementation doesn't do this" which may be fine. I think there's some uncertainty over whether the defaults should be supplied as strings or structured objects. On one hand, dropping strings into structured data definitions sucks. On the other hand, the defaults need to be rendered in the --help output, and right now we implicitly rely on __str__ to produce something useful. In most cases it does, since most cases are just integers or floats, but this might not always be the case. Addressing this might also be outside the scope of this ticket.

[glyph]

For what it's worth, I strongly agree with exarkun's point about dealing with documentation in the same pass as type requirements. It might seem like bundling in too much to the same issue, but if it is not going to be done here it should at least be designed, since this is really a significant change to the way users are expected to interact with usage.Options. I also think it is a big improvement, but it will introduce a lot more more confusion if there are two big option-parsing architectural upgrades in subsequent Twisted versions, one for documentation and one for type conversion, than if we hold off and bundle them into one change.

Perhaps we should have separate tickets for that and defaults, but I believe they should all be in the same changeset.

[therve]

I don't see why the documentation of options is not enough for dealing with the type. The situation is not any worse than before, you just have a new feature that does the coerce for you. But we may add some examples of Mixin that do the right thing for the user (like ReactorSelectionMixin).

But, if we want to add automatic documentation, here are the possibilities I see:

• Use __name__ of the coerce function. Not really meaningful
• Use __doc__ of the coerce function. For generic function like int it's pretty stupid.
• Use __str__ or __repr__ of the coerce function. Again, generally not meaningful.

So, what's the other options?

[exarkun]

These options are all examples of expanding the interface required of the coerce function. Functions already have all these attributes, so they're very minor expansions. :) Taking the same idea further, though, we would probably arrive at something like requiring some attribute which functions don't already have. Another idea in that direction would be to formalize the interface with a z.i and adapt, allowing builtins like int to play nicely.

I didn't see the version of the code which existed when itamar made his comment, but I guess it required an object with a method to do the coerce, rather than just taking a function? I hate to say it, but that might have been better, at least in this regard, since it would let us add another method or attribute for the documentation. :)

Mostly just thinking out loud here, not sure any of this is necessarily the way to take this.

[therve]

No, the original version only accept aliases 'int' and 'float' and made the coerce accordingly.

The idea of adding an attribute is probably not so bad. We'll have to define some common coerce functions to show how it could be used though.

[therve]

I tried something with a __coercedoc__ attribute, create an example with a portCoerce function, and used it in twisted.mail. That doesn't seem too bad.

[exarkun]

Cool. Seems decent. I'd suggest a couple re-arrangements:

• Drop the leading and trailing underscores on the attribute. I'd also like to suggest a better name than "coercedoc" but I can't really think of one.
• In parseOptions, get rid of the isinstance(..., CoerceParameter) and in _gather_parameters change CoerceParameter(...) to CoerceParameter(...).dispatch
• Add a method to CoerceParameter for getting the function's extra doc attribute and call that in docMakeChunks, instead of reaching all the way to d.coerce.__coercedoc__

Later on maybe we can make CoerceParameter's API more public and allow alternate ways to supply functions and documentation (if we can come up with any better ways).

[therve]

I added a doc attribute to CoerceParameter, but I have to store the CoerceParameter instance in the _dispatch attribute, so that I can have access this attribute later in docMakeChunks (unless I missed something).

[therve]

Closed in r20100.

[therve]

Sorry, wrong ticket.

[radix]

This looks ok, except for the strange underscores around _coercedoc_. I suggest renaming it to coerceDoc.

+ self.failUnlessEqual(self.usage.optsfooint?, 392) + self.assert_(isinstance(self.usage.optsfooint?, int)) + self.failUnlessEqual(self.usage.optsfoofloat?, 4.23) + self.assert_(isinstance(self.usage.optsfoofloat?, float))

hmm... That seems strange, given you're comparing with equality there already, but I guess you're protecting against the value being some number-like object that compares true with those values but isn't of the same type.. Ok, fair enough.

I also don't think it's worth explicitly testing what happens when you pass a non-callable as the coerce function, but whatever, no blocker.

+ The .parent pointer is correct even when the same Options class is used twice.

You changed that line's docstring formatting but didn't rewrap it to 80 columns. There are some other >80 char lines in the diff, can you make sure you've got them all wrapped?

I also suggest changing the port's coercion documentation from "Require an int between 0 and 65535" to "Must be an int between 0 and 65535". The "Require" form looks a bit strange in the "twistd mail --help" output.

Can you also please update the docstring for Options, where it describes optParameters, to mention the new coercion thing.

Thanks a lot for updating the howto, but can you also describe the _coercedoc_ (coerceDoc) attribute there?

[therve]

Replying to radix:

This looks ok, except for the strange underscores around _coercedoc_. I suggest renaming it to coerceDoc.

Yep looks better.

hmm... That seems strange, given you're comparing with equality there already, but I guess you're protecting against the value being some number-like object that compares true with those values but isn't of the same type.. Ok, fair enough.

Yes, as 1.0 == 1, I wanted to be explicit here.

I also don't think it's worth explicitly testing what happens when you pass a non-callable as the coerce function, but whatever, no blocker.

There have been move back and forth for what should be done here, so I think knowing what the behavior will be is useful.

You changed that line's docstring formatting but didn't rewrap it to 80 columns. There are some other >80 char lines in the diff, can you make sure you've got them all wrapped?

Ok I wrapped both files at 80.

I also suggest changing the port's coercion documentation from "Require an int between 0 and 65535" to "Must be an int between 0 and 65535". The "Require" form looks a bit strange in the "twistd mail --help" output.

Done.

Can you also please update the docstring for Options, where it describes optParameters, to mention the new coercion thing.

Done.

Thanks a lot for updating the howto, but can you also describe the _coercedoc_ (coerceDoc) attribute there?

I've added a simple example.

Thanks!

[radix]

Just a few minor English nits

options.xhtml

+ <p>The coerce function may have a coerceDoc attribute, which content will + be printed after the documentation of the option.

That was almost an awesome use of slightly archaic English, but not quite. You probably want "coerceDoc attribute, the content of which will be printed..."

usage.py

+ A coerce function can also be specified as last element: it will be called + with the value passed in argument of the program, and should return the + value that will be stored as option. This function can have a C{coerceDoc} + attribute, that will be happen to the documentation of the option.

Here's a better way to write that:

"A coerce function can also be specified as the last element: it will be called with the argument and should return the value that will be stored for the option. This function can have a C{coerceDoc} attribute which will be appended to the documentation of the option."

+1: if you fix these two minor issues please just merge it.

Thanks a lot.

[therve]

(In [20243]) Merge usage-typed-739

Author: therve Reviewers: radix, exarkun Fixes #739

Add type enforcement to twisted.python.usage, through a new element of the optParameters array called when options are stored. This coercion can also be specifically documented via a custom attribute, coerceDoc. To illustrate this support, a portCoerce function has been added to t.p.usage and used in t.mail.tap.