classification
Title: Using consistent naming for arguments of "call" functions (C API)
Type: enhancement Stage: patch review
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.smith, ncoghlan, python-dev, r.david.murray, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2016-11-30 11:13 by vstinner, last changed 2016-12-21 09:47 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
call.patch vstinner, 2016-11-30 11:13 review
rename.patch vstinner, 2016-12-06 11:07 review
cleanup.patch vstinner, 2016-12-06 17:45
rename_pushed.patch vstinner, 2016-12-06 23:24
cleanup-2.patch vstinner, 2016-12-08 16:09 review
Messages (28)
msg282078 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-11-30 11:13
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
msg282081 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-11-30 12:27
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)
msg282085 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-11-30 15:33
Serhiy: Can you please elaborate "Other changes looks not well justified too."?
msg282115 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2016-11-30 23:34
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.
msg282146 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-01 08:46
> * 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 issue18697.
msg282431 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-05 16:38
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.
msg282482 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2016-12-05 22:57
I withdraw my concern. I was thinking it would affect the caller, but of course it won't. Apologies.
msg282490 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-06 01:09
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.
msg282517 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-06 11:07
rename.patch: Big patch to rename function parameters.
msg282543 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-06 12:04
Added comments on Rietveld.

I think we can keep "func" instead of "callable" in the source code for decreasing the number of changes.
msg282551 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-06 15:32
New changeset e6ad41a3f3bd by Victor Stinner in branch 'default':
Uniformize argument names of "call" functions
https://hg.python.org/cpython/rev/e6ad41a3f3bd
msg282552 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-06 15:36
I pushed a change which is rename.patch with fixes on comments in abstract.h. It seems like comments were no more revelant or moved to random places. I moved many comments, rewrote some of them, etc. Some comments were wrong like "This is the equivalent of the Python expression: o.method(args)", it's "o.method(*args)" in fact. Serhiy catched this mistake, it should now be fixed.

I chose to push this large change, instead of having another round of review, because it's tricky to handle such large changes. I suggest to push following changes if you still see issues.

I also wanted to push it quickly because I have pending changes modifying the same parts of the code in my queue :-)

Thanks for reviews Serhiy! Very useful, as usual ;-)


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

I prefer to "fix" it as well, the purpose of the issue is to uniformize everything: .c files, .h files and .rst documentation.
msg282554 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-12-06 16:02
"Uniformize" isn't really an English word :)
msg282555 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-06 16:25
Oh, you have mixed up comments! In headers descriptions are written after function declarations. 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.

You should either revert your changes or change *all* headers by moving *all* descriptions above corresponding declarations. That would be very large patch, and I doubt it will be approved.

Please don't make such large changes without a review.
msg282559 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-06 17:38
New changeset 69c7451c3ec1 by Victor Stinner in branch 'default':
Issue #28838: Fix weird indentation of abstract.h
https://hg.python.org/cpython/rev/69c7451c3ec1
msg282561 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-06 17:45
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.
msg282568 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-06 19:37
> 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.
msg282576 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-06 23:24
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.
msg282714 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-08 16:09
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.
msg283251 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-15 08:15
New changeset 654ec6ed3225 by Victor Stinner in branch 'default':
Issue #28838: Cleanup abstract.h
https://hg.python.org/cpython/rev/654ec6ed3225
msg283255 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-15 08:29
Victor, it would be worth to discuss this change on Python-Dev.
msg283257 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-15 08:37
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 #28977. I think that we should then synchronize .rst doc and .h comments to make them consistent again (and maybe update the doc).
msg283277 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-15 10:23
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
msg283280 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-15 10:38
Thanks Victor.
msg283403 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-16 15:00
I consider that the cleanup itself is done, so I close the issue. For further enhancements, I will open new issues.
msg283613 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-12-19 12:12
New changeset 3ab0a6692e25 by Victor Stinner in branch 'default':
abstract.h: remove long outdated comment
https://hg.python.org/cpython/rev/3ab0a6692e25
msg283722 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-21 08:29
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.
msg283731 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-12-21 09:47
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.
History
Date User Action Args
2016-12-21 09:47:07vstinnersetmessages: + msg283731
2016-12-21 08:29:46serhiy.storchakasetmessages: + msg283722
2016-12-19 12:12:47python-devsetmessages: + msg283613
2016-12-16 15:00:02vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg283403
2016-12-15 10:38:54serhiy.storchakasetmessages: + msg283280
2016-12-15 10:23:12vstinnersetmessages: + msg283277
2016-12-15 08:37:58vstinnersetmessages: + msg283257
2016-12-15 08:29:04serhiy.storchakasetmessages: + msg283255
2016-12-15 08:15:28python-devsetmessages: + msg283251
2016-12-08 16:09:59vstinnersetfiles: + cleanup-2.patch

messages: + msg282714
2016-12-06 23:24:36vstinnersetfiles: + rename_pushed.patch

messages: + msg282576
2016-12-06 19:37:13serhiy.storchakasetmessages: + msg282568
2016-12-06 17:45:21vstinnersetfiles: + cleanup.patch

messages: + msg282561
2016-12-06 17:38:18python-devsetmessages: + msg282559
2016-12-06 16:25:03serhiy.storchakasetmessages: + msg282555
2016-12-06 16:02:53r.david.murraysetnosy: + r.david.murray

messages: + msg282554
title: Uniformize argument names of "call" functions (C API) -> Using consistent naming for arguments of "call" functions (C API)
2016-12-06 15:36:48vstinnersetmessages: + msg282552
2016-12-06 15:32:03python-devsetnosy: + python-dev
messages: + msg282551
2016-12-06 12:04:46serhiy.storchakasetmessages: + msg282543
2016-12-06 11:07:40vstinnersetfiles: + rename.patch

messages: + msg282517
2016-12-06 01:09:09ncoghlansetmessages: + msg282490
2016-12-05 22:57:26eric.smithsetmessages: + msg282482
2016-12-05 16:38:17vstinnersetmessages: + msg282431
2016-12-01 08:46:26serhiy.storchakasetmessages: + msg282146
stage: patch review
2016-11-30 23:34:52eric.smithsetnosy: + eric.smith
messages: + msg282115
2016-11-30 15:33:04vstinnersetmessages: + msg282085
2016-11-30 12:27:13ncoghlansetnosy: + ncoghlan
messages: + msg282081
2016-11-30 11:14:06vstinnersettitle: Uniformize argument names of "call" functions -> Uniformize argument names of "call" functions (C API)
2016-11-30 11:13:57vstinnercreate