classification
Title: Argument Clinic: add support for __init__
Type: enhancement Stage: resolved
Components: Demos and Tools Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: larry, python-dev, rmsr, serhiy.storchaka, zach.ware
Priority: normal Keywords:

Created on 2014-01-18 11:43 by serhiy.storchaka, last changed 2014-01-22 03:13 by larry. This issue is now closed.

Files
File name Uploaded Description Edit
larry.argument.clinic.init.and.new.patch.1.txt larry, 2014-01-19 04:39
larry.argument.clinic.init.and.new.patch.2.txt larry, 2014-01-19 06:10 review
Messages (13)
msg208395 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-18 11:43
Currently Argument Clinic doesn't support the __init__ methods, mainly because the C implementation of this method should return int, not PyObject *. In some cases it is possible to wrap a function generated by Argument Clinic (as in Modules/_pickle.c). But not always, because the __init__ method is called with the self, args, and kwargs arguments, while Argument Clinic can generate function without kwargs.
msg208406 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-18 17:41
Oh!  I thought it worked fine, because I've seen people convert both an __init__ and a __new__.  But I guess they wrapped them.

It's an easy change, I can try to do that today.
msg208412 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-18 19:36
>  * As is probably expected, __init__ and __call__ procs
> can't be converted.

I should have a fix in for __init__ and __call__ later today.  (Issue #20294.)
For the record, you could convert them, you just had to wrap the parsing function
to deal with the return type difference, and *args / **kwargs might be tricky too.

>     * sqlite3.Connection.{execute, executemany, executescript} don't use
>       PyArg_Parse*.

In the next few days I'm going to add support for "*args" and "**kwargs", and
then you'll be able to convert these functions.  (Issue #20291.)  Again, even
if Argument Clinic doesn't do any parsing, it's still helpful to have it
generate signatures.


>     * The clinic version of 'sqlite3.adapt' has an argument string of "O|OO".
>       The first optional parameter defaults to 'pysqlite_PrepareProtocolType'
>       in C and 'sqlite3.ProtocolType' in Python.  However, I don't know how to
>       represent the Python value because Python default values are 'eval'd by
>       clinic *and* the 'sqlite3' module is not imported for the eval.

You'll be able to do this soon:

parameter: object(c_default='pysqlite_PrepareProtocolType') = ProtocolType

This feature isn't checked in yet, I'm waiting for a review.  It's part of
#20226, which I guess you already noticed.

>  * The clinic version of 'sqlite3.Cursor.fetchmany' has an arguments string
>    of "|i".  The first parameter defaults to
>    '((pysqlite_Cursor *)self)->arraysize' in C and 'self.arraysize' in
>    Python.  I don't know how to express the Python initialization.

You can't.  How about you use a default of -1 and then:

  if (maxrows == -1)
      maxrows = self->arraysize

>  * I didn't convert 'sqlite3.Cursor.setinputsizes' and
>    'sqlite3.Cursor.setoutputsize' because they share a docstring and
>    simple no-op method def.

I'd prefer it if you converted them anyway.  Converting them means they'll
have signature information which is an unconditional good.

>  * I didn't convert 'sqlite.connect' because it would have required
> packaing the arguments back up to pass to 'PyObject_Call'.

Once I add the ability to pass in *args and **kwargs, you'll be able to
convert sqlite.connect.  

I think I responded to all your other comments when I reviewed the patch.

Thanks!
msg208413 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-18 19:37
Oops, sorry, that last comment was intended for a different issue.  Please ignore, and sorry for the spam!
msg208444 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-19 04:39
Here's a patch.  Wasn't as easy as I thought, it kind of took all day.

Changes include:

* __init__ and __new__ always take kwargs.
  * if the function signature doesn't accept keyword arguments,
    it calls _PyArg_NoKeywords().
* __init__ returns int, not PyObject *.
* __init__ and __new__ should support all argument parsing
  scenarios (unpack tuple, positional only, etc).

Pre-exiting behavior:
* The C function basename chosen for __new__ is just the name of
  the class, it doesn't end in __new__.
* The methoddef #define is suppressed.
msg208451 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-19 05:59
Rietveld doesn't like the patch and I'm having a hard time finding a changeset on which I can get it to apply cleanly.  May I suggest avoiding `--git` on `hg diff` unless copying/moving/renaming a file?
msg208453 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-19 06:10
Whoops, that doesn't apply cleanly.  Here's an updated patch.
msg208458 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-01-19 07:20
Output looks good and I didn't see anything that scared me in the patch, though I can't pretend to fully understand 100% of it.  Builds on Windows, pickle tests pass, and nothing else is affected (that's checked in so far).  Looks good to me, as far as I can see.
msg208459 - (view) Author: Ryan Smith-Roberts (rmsr) * Date: 2014-01-19 07:23
I have reviewed this as best I am able. I'll be honest that a lot of clinic.py makes my eyes cross; I'm used to webdev templating, which is inverted from AC (flow control inside the template). Not complaining, it's a complicated subject. I do like the centralization of the templates.

I didn't see anything that jumped out at me. I tried out the new conversions on socketmodule and like the output, and none of the existing conversions mutated. So LGTM!

It took me a while to figure out how to make __new__ a class method though, decorator support should probably be documented (or else a hint like '__new__ must be a @classmethod!')
msg208460 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-19 07:50
New changeset 2e32462e4832 by Larry Hastings in branch 'default':
Issue #20294: Argument Clinic now supports argument parsing for __new__ and
http://hg.python.org/cpython/rev/2e32462e4832
msg208461 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-19 07:51
Checked in.  Thanks for bringing the problem to my attention, Serhiy, and thanks for the reviews you two!

rmsr: If I had a stronger background in templating, maybe Argument Clinic would be cleaner inside.  As it is I'm beating the problem to death with sheer force of will.
msg208497 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-19 20:13
Thank you Larry, now I can use Argument Clinic in the __init__ methods. But there is one problem.

Docstring generated for the __init__ method contains the "__init__" name in the signature. Therefore it can't be used as class docstring. On other hand, the compiler complains of not used __init__ docstring.
msg208739 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-22 03:13
That problem will be irrelevant, once builtin classes support signatures (#20189).
History
Date User Action Args
2014-01-22 03:13:07larrysetmessages: + msg208739
2014-01-19 20:13:31serhiy.storchakasetmessages: + msg208497
2014-01-19 07:51:59larrysetstatus: open -> closed
resolution: fixed
messages: + msg208461

stage: resolved
2014-01-19 07:50:46python-devsetnosy: + python-dev
messages: + msg208460
2014-01-19 07:23:07rmsrsetnosy: + rmsr
messages: + msg208459
2014-01-19 07:20:34zach.waresetmessages: + msg208458
2014-01-19 06:10:46larrysetfiles: + larry.argument.clinic.init.and.new.patch.2.txt

messages: + msg208453
2014-01-19 05:59:29zach.waresetnosy: + zach.ware
messages: + msg208451
2014-01-19 04:40:03larrysetfiles: + larry.argument.clinic.init.and.new.patch.1.txt

messages: + msg208444
2014-01-18 19:37:55larrysetmessages: + msg208413
2014-01-18 19:36:52larrysetmessages: + msg208412
2014-01-18 17:41:45larrysetassignee: larry
messages: + msg208406
2014-01-18 11:47:48serhiy.storchakalinkissue20193 dependencies
2014-01-18 11:43:26serhiy.storchakacreate