Using consistent naming for arguments of "call" functions (C API)

@ncoghlan

BPO	28838
Nosy	@ncoghlan, @vstinner, @ericvsmith, @bitdancer, @serhiy-storchaka
Files	call.patch rename.patch cleanup.patch rename_pushed.patch cleanup-2.patch

^{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 = None
closed_at = <Date 2016-12-16.15:00:02.606>
created_at = <Date 2016-11-30.11:13:57.694>
labels = ['interpreter-core', 'type-feature', '3.7']
title = 'Using consistent naming for arguments of "call" functions (C API)'
updated_at = <Date 2016-12-21.09:47:07.086>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-12-21.09:47:07.086>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2016-12-16.15:00:02.606>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2016-11-30.11:13:57.694>
creator = 'vstinner'
dependencies = []
files = ['45702', '45776', '45777', '45779', '45798']
hgrepos = []
issue_num = 28838
keywords = ['patch']
message_count = 28.0
messages = ['282078', '282081', '282085', '282115', '282146', '282431', '282482', '282490', '282517', '282543', '282551', '282552', '282554', '282555', '282559', '282561', '282568', '282576', '282714', '283251', '283255', '283257', '283277', '283280', '283403', '283613', '283722', '283731']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'vstinner', 'eric.smith', 'r.david.murray', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue28838'
versions = ['Python 3.7']

See python-dev thread about my changeset 7efddbf1aa70:
https://mail.python.org/pipermail/python-dev/2016-November/146885.html

Serhiy asked me to revert it. It's now reverted by the revision 20bb8babc505. So, here is my change as a patch to review it.

See the thread for my rationale on argument names.

--

Uniformize argument names of "call" functions

Callable object: callable, o, callable_object => func
Object for method calls: o => obj
Method name: name or nameid => method

Cleanup also the C code:

Don't initialize variables to NULL if they are not used before their first
assignement
Add braces for readability

Independently of anything else, any changes made to the parameter names in the implementation should be reflected in the API docs: https://docs.python.org/3/c-api/object.html#c.PyCallable_Check

A style guide addition to PEP-7 would also be useful as a record of the preferred naming scheme.

That said, one of the problems you're up against inside the C code base is that we really do have some code evaluation APIs that only work with true Python functions, while a abstract APIs handle arbitrary callables. Reserving "func" for the former cases helps make it clear which is which.

By contrast, end user code (and even the standard library) can usually get away with saying "func" even when they mean "arbitrary callable" without causing confusion, as there's only the one Python-level call syntax.

So perhaps one way to start here would be to propose an addition to https://www.python.org/dev/peps/pep-0007/#naming-conventions specifically for dealing with callable parameters in APIs that covers parameters that are:

arbitrary callables
specifically a synchronous Python function
method names (whether as const char *, PyObject *, or _Py_Identifier)

Serhiy: Can you please elaborate "Other changes looks not well justified too."?

Isn't this just a lot of churn for not a lot of benefit? I'm all for consistency, but you'll break patches on the bug tracker and externally maintained patches.

Callable object: callable, o, callable_object => func

The original names better describes the argument type in functions like PyObject_Call(). It works not just with functions, but with arbitrary callables. For example it is used with type objects for creating instances.

Method name: name or nameid => method

The parameter is not a *method object*, but a method *name*. You have to pass an object and a method names to call a method.

Serhiy: Can you please elaborate "Other changes looks not well justified too."?

Renaming obj to arg0 looks questionable to me. I would rather rename func to method (this is a true unbound method, not method name as in PyObject_CallMethod()).

It would be better to uniformize parameter names in the documentation (and perhaps in headers) and left sources untouched unless we rewrite particular functions. Otherwise it looks as code churn. It would be nice to formalize naming rules in PEP-7.

See also bpo-18697.

So, what do you think of renaming argument like:

Replace "callable_object" and "func" with "callable"
Replace "method" with "name"
Rename "o" to "obj" (ex: in "PyCallable_Check(PyObject *o)")

I prefer "callable_object" over "callable" because it's shorter. Some functions already use "callable" (and so others use "callable_object").

Eric: ""Isn't this just a lot of churn for not a lot of benefit? I'm all for consistency, but you'll break patches on the bug tracker and externally maintained patches."

I'm not aware of any pending patch on "call" functions like PyObject_Call. I think that I am the latest who modified these functions. I'm working on a new patch serie to add support for fast calls in tp_call. That's why I would prefer to uniformize argument names first, before rebasing my large patch.

I withdraw my concern. I was thinking it would affect the caller, but of course it won't. Apologies.

As long as the C API docs also get updated, +1 for:

'o' -> 'obj'
'callable_object' -> 'callable'
'func' -> 'callable' (when the function actually accepts an arbitrary callable)
'method' -> 'name' (assuming 'Method' appears in the function name)

Documenting those naming conventions in PEP-7 would also still be desirable.

rename.patch: Big patch to rename function parameters.

Added comments on Rietveld.

I think we can keep "func" instead of "callable" in the source code for decreasing the number of changes.

New changeset e6ad41a3f3bd by Victor Stinner in branch 'default':
Uniformize argument names of "call" functions
https://hg.python.org/cpython/rev/e6ad41a3f3bd

I pushed another large patch to "fix" the indentation of abstract.h. I would prefer to also have this "reindent" change on review, but our reviewing tool doesn't support patch series :-(

Attached cleanup.patch reformats deeply the header file to use the same style for all comments, use the same style used in other Python headers. IMO it makes the header file more consistent with other header files and it makes the file more readable.

R. David Murray (r.david.murray):

Uniformize" isn't really an English word :)

Oh, it's a frenglish word in this case :-) Good to know.

Serhiy Storchaka: "You have moved some comments and now some descriptions are placed below the corresponding declaration, and some descriptions are placed above. Now it is not clear to what function every comment is related."

It don't think that my change makes abstract.h worse, some comments were far from their declaration, and there is was no unique style for comments. It's a mess.

Serhiy: "You should either revert your changes or change *all* headers by moving *all* descriptions above corresponding declarations."

My plan is to rewrite completely abstact.h, this file annoys me since many months :-)

"That would be very large patch, and I doubt it will be approved."

Why not?

"Please don't make such large changes without a review."

You reviewed rename.patch, I only made tiny changes after this patch. As I explained, it's hard to work on such large patch.

It don't think that my change makes abstract.h worse, some comments were far
from their declaration, and there is was no unique style for comments. It's
a mess.

Seems most of comments that did not follow common style are your comments to
"fast call" functions. It would be better to fix only that comments.

"That would be very large patch, and I doubt it will be approved."
Why not?

Because it breaks "hg annotation" and makes harder researching the history of
the file. That harms the workflow of other developers. Ask on Python-Dev before
making such larger and questionable changes.

You reviewed rename.patch, I only made tiny changes after this patch.

Reviewed patch did not contain committed changes. That changes are not tiny.

Serhiy:

Seems most of comments that did not follow common style are your comments to "fast call" functions. It would be better to fix only that comments.

I would like to modify abstract.h because its weird style annoy me for years. I would like to make the header consistent with all other headers.

I checked unicodeobject.h, code.h, frameobject.h, floatobject.h, etc.: comments are always before declaration.

About "fast calls" in abstract.h, sorry, it's my fault. When I added new code, I used the same style than all other files. I didn't notice that abstract.h has a special style. That's why I would like to make it consistent again.

Serhiy:

Reviewed patch did not contain committed changes.

You wrote on the review that a comment is misplaced: "The comment is related to PyObject_Call() declared 85 lines above."

That's why I took the opportunity of a refactoring change to fix comments.

Serhiy:

That changes are not tiny.

Between rename.patch and the pushed change e6ad41a3f3bd, I only moved 5 comments: see attached rename_pushed.patch which is the diff between the two patches.

For me, it was obvious that the comment must be before the declaration, since it is the style used in all other Python headers. Sorry, I didn't expect that you didn't want to change that. I misunderstood your comment on the review.

cleanup-2.patch: Rebase cleanup.patch.

This patch is the last part of my work to cleanup Include/abstract.h to make it more consistent with other header files.

New changeset 654ec6ed3225 by Victor Stinner in branch 'default':
Issue bpo-28838: Cleanup abstract.h
https://hg.python.org/cpython/rev/654ec6ed3225

Victor, it would be worth to discuss this change on Python-Dev.

I pushed cleanup-2.patch (after a minor rebase). Sorry for breaking "hg annotation" on Include/abstract.h: I hope that it will be less annoying that the giant "replace tabs with space" made by Antoine Pitrou a few years ago which modified almost all .c and .h files (my change only changes a single file, abstract.h). If you have large pending patch series which modify this file, ping me, I will help you to rebase them.

Sorry, I didn't expect this issue to be so painful. I hope that abstract.h is now more readable, easier to maintain (no more surprising indentation!), and have better comments. I now started to cleanup the documentation of functions of the "PyObject_Call() family": see my first issue bpo-28977. I think that we should then synchronize .rst doc and .h comments to make them consistent again (and maybe update the doc).

Serhiy: "Victor, it would be worth to discuss this change on Python-Dev."

Done: https://mail.python.org/pipermail/python-dev/2016-December/146999.html

Thanks Victor.

I consider that the cleanup itself is done, so I close the issue. For further enhancements, I will open new issues.

New changeset 3ab0a6692e25 by Victor Stinner in branch 'default':
abstract.h: remove long outdated comment
https://hg.python.org/cpython/rev/3ab0a6692e25

Since no one on Python-Dev has objections, I'm fine with this too. I was opposed not so much to the changes themselves, as to making large changes to one of basic files that could affect everybody without wider discussion. Excuse me if I looked unfriendly.

Serhiy Storchaka added the comment:

Since no one on Python-Dev has objections, I'm fine with this too. I was opposed not so much to the changes themselves, as to making large changes to one of basic files that could affect everybody without wider discussion. Excuse me if I looked unfriendly.

No problem. Well, it's just reformating. It doesn't change the API for
example. It should only impact people maintaining large external
patches on Python 3.6. I'm not aware of any pending patch modifying
abstract.h. And as I wrote, if there is such patch, contact me, I will
help to rebase your change.

Navigation Menu

Repository navigation

Using consistent naming for arguments of "call" functions (C API) #73024

5 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Using consistent naming for arguments of "call" functions (C API) #73024

Description

Activity

vstinner commented on Nov 30, 2016

ncoghlan commented on Nov 30, 2016

vstinner commented on Nov 30, 2016

ericvsmith commented on Nov 30, 2016

serhiy-storchaka commented on Dec 1, 2016

vstinner commented on Dec 5, 2016

ericvsmith commented on Dec 5, 2016

ncoghlan commented on Dec 6, 2016

vstinner commented on Dec 6, 2016

serhiy-storchaka commented on Dec 6, 2016

python-dev commented on Dec 6, 2016

5 remaining items

vstinner commented on Dec 6, 2016

serhiy-storchaka commented on Dec 6, 2016

vstinner commented on Dec 6, 2016

vstinner commented on Dec 8, 2016

python-dev commented on Dec 15, 2016

serhiy-storchaka commented on Dec 15, 2016

vstinner commented on Dec 15, 2016

vstinner commented on Dec 15, 2016

serhiy-storchaka commented on Dec 15, 2016

vstinner commented on Dec 16, 2016

python-dev commented on Dec 19, 2016

serhiy-storchaka commented on Dec 21, 2016

vstinner commented on Dec 21, 2016

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions