On Fri, Jun 22, 2012 at 12:10 PM, Yury Selivanov
<yselivanov...@gmail.com> wrote:
> Guido,
>
> On 2012-06-22, at 2:52 PM, Guido van Rossum wrote:
>
>> This looks great, much better than the version I reviewed half a year
>> ago! Thanks you and others (especially Yuri) for all your efforts in
>> guiding the discussion and implementing as the discussion went along;
>> also thanks to Nick for participating to intensely.
>>
>> Quick review notes:
>>
>> (1) I don't like APIs that require you to use hasattr(), which you use
>> for return_annotation. Since you have Signature.empty to remove it,
>> why not set it to Signature.empty when not set? Ditto for other values
>> that are currently not set when they don't apply (Parameter.empty I
>> think.)
>
> I think that if a function lacks an annotation, that should be reflected
> in the same way for its signature.
>
> Currently:
>
> if hasattr(signature, 'return_annotation'):
>
> If we use Signature.empty:
>
> if signature.return_annotation is not signature.empty:
>
> So (in my humble opinion) it doesn't simplify things too much.
But it is much more of a pain if someone just wants to pass the value
on or print it. Instead of
print(sig.return_annotation)
you'd have to write
if hasattr(sig, 'return_annotation'):
print(sig.return_annotation)
else:
print('empty')
In general hasattr() (or getattr() with a 3rd arg) has a code smell;
making it a required part of an API truly stinks IMO.
> And also you can use 'try .. except AttributeError .. else' blocks,
> which make code even more readable.
No, try/except blocks make code *less* readable.
> All in all, I don't have a very strong opinion in this topic.
But I do. :-)
> 'empty' will also work. When python-dev collectively decided to
> go with missing attributes, 'empty' didn't yet exist (we added
> it with 'replace()' methods).
>
> If you think that using 'empty' is better, we can add that to the PEP.
Yes, please do.
>> (2) Could use an example on how to remove and add parameters using replace().
>
> You have to build a new list of parameters and then pass it to 'replace()'.
> Example (from the actual signature() function implementation):
>
> if isinstance(obj, types.MethodType):
> # In this case we skip the first parameter of the underlying
> # function (usually `self` or `cls`).
> sig = signature(obj.__func__)
> return sig.replace(parameters=tuple(sig.parameters.values())[1:])
>
>> (3) You are using name(arg1, *, arg2) a lot. I think in most cases you
>> mean for arg2 to be an optional keyword arg, but this notation doesn't
>> convey that it is optional. Can you clarify?
>
> Yes, I meant optional. Would 'name(arg1, *, [arg2])' be better?
Hardly, because that's not valid syntax. I'd write name(arg1, *,
arg2=<default>).
>> (4) "If the object is a method" -- shouldn't that be "bound method"?
>> (Unbound methods are undetectable.) Or is there some wider definition
>> of method? What does it do for static or class methods?
>
> Yes, it should be "If the object is a bound method". We'll fix this
> shortly.
Great.
> classmethod as a descriptor returns a BoundMethod (bound to the class),
> staticmethod returns the original unmodified function, so both of
> them are supported automatically.
Oh, great. IIRC it wasn't always like that. Maybe just add this to the
PEP as a note?
>> (5) Too bad there's no proposal for adding signatures to builtin
>> functions/methods, but understood.
>>
>> Of these, only (1) is a blocker for PEP acceptance -- I'd either like
>> to see this defended vigorously (maybe it was discussed? then please
>> quote, I can't go back and read the threads) or changed.
>>
>> Otherwise it looks great!
>>
>> --Guido
>
> Thanks!
Thank *you* for your hard work!
--
--Guido van Rossum (python.org/~guido)
_______________________________________________
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
