Argument Clinic: add support for __init__

[serhiy-storchaka]

BPO	20294
Nosy	@larryhastings, @zware, @serhiy-storchaka
Files	larry.argument.clinic.init.and.new.patch.1.txt larry.argument.clinic.init.and.new.patch.2.txt

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/larryhastings'
closed_at = <Date 2014-01-19.07:51:59.784>
created_at = <Date 2014-01-18.11:43:26.038>
labels = ['type-feature']
title = 'Argument Clinic: add support for __init__'
updated_at = <Date 2014-01-22.03:13:07.590>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2014-01-22.03:13:07.590>
actor = 'larry'
assignee = 'larry'
closed = True
closed_date = <Date 2014-01-19.07:51:59.784>
closer = 'larry'
components = ['Demos and Tools']
creation = <Date 2014-01-18.11:43:26.038>
creator = 'serhiy.storchaka'
dependencies = []
files = ['33537', '33542']
hgrepos = []
issue_num = 20294
keywords = []
message_count = 13.0
messages = ['208395', '208406', '208412', '208413', '208444', '208451', '208453', '208458', '208459', '208460', '208461', '208497', '208739']
nosy_count = 5.0
nosy_names = ['larry', 'rmsr', 'python-dev', 'zach.ware', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue20294'
versions = ['Python 3.4']

[serhiy-storchaka]

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.

[larryhastings]

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.

[larryhastings]

• 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 bpo-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 bpo-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
bpo-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!

[larryhastings]

Oops, sorry, that last comment was intended for a different issue. Please ignore, and sorry for the spam!

[larryhastings]

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.

[zware]

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?

[larryhastings]

Whoops, that doesn't apply cleanly. Here's an updated patch.

[zware]

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.

[rmsr]

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!')

[python-dev]

New changeset 2e32462e4832 by Larry Hastings in branch 'default':
Issue bpo-20294: Argument Clinic now supports argument parsing for __new__ and
http://hg.python.org/cpython/rev/2e32462e4832

[larryhastings]

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.

[serhiy-storchaka]

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.

[larryhastings]

That problem will be irrelevant, once builtin classes support signatures (bpo-20189).