classification
Title: Argument Clinic should add markers for humans
Type: behavior Stage: resolved
Components: Build, Demos and Tools Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: larry Nosy List: gvanrossum, larry, ncoghlan, pitrou, python-dev, serhiy.storchaka, skrah
Priority: normal Keywords:

Created on 2013-11-22 20:04 by pitrou, last changed 2014-01-07 22:25 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
larry.change.clinic.markers.diff.1.txt larry, 2014-01-07 14:35 review
Messages (24)
msg203852 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-11-22 20:04
I was reviewing a patch by Larry and I started commenting on some rather tasteless code, until I realized it was generated by Argument Clinic.

It would be nice if Argument Clinic added some markers, such as:

/* Start of code generated by Argument Clinic */

/* End of code generated by Argument Clinic */
msg203858 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-22 20:27
You find the big
/*[clinic checksum: b6ded2204fd0aab263564feb5aae6bac840b5b94]*/
marker insufficient?

Perhaps this is simply something we will quickly get used to.  How about we let this sit for a while and see what other people think.

p.s. I accept your critique of Clinic's autogenerated code.
msg203864 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-11-22 20:31
> You find the big
> /*[clinic checksum: b6ded2204fd0aab263564feb5aae6bac840b5b94]*/
> marker insufficient?

Well, it doesn't say anything except "checksum", and it doesn't seem
geared at humans (except perhaps those with a hex calculator in their
head :-)).

Clinic-generated code looks at first sight like code that would be
written by a human, so it's easy to get miffed.
msg203869 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2013-11-22 20:52
I think it's possible to get used to the markers.  However, to bikeshed a
little, I would prefer "preprocessor" instead of "clinic", since jokes
tend to wear off if one sees then too often.
msg203899 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-11-22 22:05
> However, to bikeshed a little, I would prefer
> "preprocessor" instead of "clinic", since jokes
> tend to wear off if one sees then too often.

"Argument Clinic" is the name of the tool.  The marker /*[clinic]*/
was chosen deliberately:

* If someone says "what the hell is this" a quick online search
  should turn up the answer quickly.

* It differentiates it from /*[python]*/, which allows one to embed
  raw Python code inside files.  ("print" is redirected to the output
  section.)
msg204025 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-11-23 14:14
On 23 Nov 2013 08:05, "Larry Hastings" <report@bugs.python.org> wrote:
>
>
> Larry Hastings added the comment:
>
> > However, to bikeshed a little, I would prefer
> > "preprocessor" instead of "clinic", since jokes
> > tend to wear off if one sees then too often.
>
> "Argument Clinic" is the name of the tool.  The marker /*[clinic]*/
> was chosen deliberately:
>
> * If someone says "what the hell is this" a quick online search
>   should turn up the answer quickly.
>
> * It differentiates it from /*[python]*/, which allows one to embed
>   raw Python code inside files.  ("print" is redirected to the output
>   section.)

It also differentiates it clearly from the C preprocessor. If Guido can
name the language after a comedy troupe, Larry can certainly name the
custom code generator after one of their sketches :)

FWIW, I tend to start editing the generated code as well, so I agree this
is something for us to keep an eye on. If the habit doesn't go away, we may
want some more prominent markers.

Cheers,
Nick.

>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue19723>
> _______________________________________
msg207480 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-06 20:56
I grepped for clinic in the source code and I have a hunch why this confusing: each clinic-generated block has *three* marker comments, each containing [clinic] or [clinic checksum: ...].

TBH I can't always tell on which side of the comment the generated code sits, so I agree it would be nice if there was an additional keyword clearly indicating begin/end.

Looking more carefully it seems the pattern is

/*[clinic]
.
. (this seems to be the clinic input)
.
[clinic]*/
.
. (this seems to be generated)
.
/*[clinic checksum: da39a3ee5e6b4b0d3255bfef95601890afd80709]*/

I expect things would be clearer to the uninitiated if instead they said something like:

/*[clinic input]
.
.
.
[clinic start generated code]*/
.
.
.
/*[clinic end generated code; checksum: da39a3ee5e6b4b0d3255bfef95601890afd80709]*/

Larry, would that be easy?
msg207481 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 21:00
And for Python blocks would you suggest

   /*[python input]
   ...
   [python start generated code]*/
   ...
   /*[python end generated code; checksum: da39a3ee5e6b4b0d3255bfef95601890afd80709]*/

To answer your question: no, it wouldn't be hard.
msg207482 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-06 21:01
Yes. Then I suggest working on a patch before people get too deep into the conversion project.
msg207485 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 21:19
Why not just move most (except implementation function headers) generated code out to separate file? Right now the list of symbols in the Kate editor contains three names for every function: BINASCII_A2B_UU_METHODDEF, binascii_a2b_uu and binascii_a2b_uu_impl. This makes navigation cumbersome. Also when I search function name 
a2b_uu, I found 13 occurrences (it will be decreased to 2 or 3 if move generated code out). So search is not helpful too,
msg207486 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-06 21:35
I use Emacs, so I have no idea what would help for Kate.  I suppose Kate is programmable?  Maybe you can add some kind of filter for this purpose?

If e.g. binascii_a2b_uu_impl were moved to a different file, how would binascii_a2b_uu call it without exposing the symbol externally?
msg207487 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-01-06 21:37
> If e.g. binascii_a2b_uu_impl were moved to a different file, how would
> binascii_a2b_uu call it without exposing the symbol externally?

I think "stitching it with #include" is the only workable solution.
We already do this for e.g. stringlib.
msg207488 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-06 21:55
And the stringlib situation confuses the hell out of me already.
msg207490 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 22:06
> I use Emacs, so I have no idea what would help for Kate.  I suppose Kate is programmable?  Maybe you can add some kind of filter for this purpose?

In comparision with Emacs it is not programmable.

> If e.g. binascii_a2b_uu_impl were moved to a different file, how would binascii_a2b_uu call it without exposing the symbol externally?

No, I propose to move generated binascii_a2b_uu__doc__, BINASCII_A2B_UU_METHODDEF and binascii_a2b_uu to separate file, and left clinic input and binascii_a2b_uu_impl in main file. Then #include "binascii.clinic" should be added before the list of module functions.
msg207491 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-06 22:09
In contrast to stringlib programmers should not look in these generated files.
msg207492 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-06 23:06
Then the separate file version looks much worse to me.
msg207493 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-06 23:07
If it's source code, programmers will need to examine it from time to time.  A more important distinction imo: stringlib is type-parameterized like some sort of prehistoric C++ template specialization.  Thankfully the gunk generated by Argument Clinic is free of such dizzying technology.


Another approach to making Clinic output easier to read--which I thought I also proposed at one point--would be to save up most of the Clinic output in an "accumulator", then emit the contents of the accumulator on demand.  Clinic could probably get away with only generating a prototype for the generated function, the macro for the methoddef, and the prototype (sans semicolon) for the impl.  The bulk of the generated code, the implementation of the generated function and the docstring, would go in the "accumulator" and could be tucked away anywhere in the file you like.  I could even make it idiot-proof; if you haven't emptied the accumulator before the end of the file, it could tack the correct Clinic block on the end for you.

Here's a quick mockup of what that would look like:

/*[clinic]

    zlib.compress
        bytes: Py_buffer
            Binary data to be compressed.
        [
        level: int
            Compression level, in 0-9.
        ]
        /

    Returns compressed string.

    [clinic]*/

    #define ZLIB_COMPRESS_METHODDEF    \
        {"compress", (PyCFunction)zlib_compress, METH_VARARGS, zlib_compress__doc__},

    static PyObject *
    zlib_compress(PyModuleDef *module, PyObject *args);

    static PyObject *
    zlib_compress_impl(PyModuleDef *module, Py_buffer *bytes, int group_right_1, int level)
    /*[clinic checksum: 9f055a396620bc1a8a13d74c3496249528b32b0d]*/
    {
        PyObject *ReturnVal = NULL;
        Byte *input, *output = NULL;
        unsigned int length;
        ...


Not sure how far this suggestion ever got; I think maybe I only showed it to Stefan Krah, who said it wouldn't help his use case so I dropped it.

Any good?
msg207528 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 12:06
Antoine just suggested that, if we used this "accumulator" thing, we'd want a convention for where the generated text should go.  I actually have an answer for that: near the end, below the implementations of the module / class methods, but above the methoddef/type structures and the module init function.

My reasoning: when I navigate CPython C files implementing a module or a type, when I know what entry point I want I just search for its name.  When I don't know what I want, I jump to the end, then scroll up until I find the name in the init function or the structures.  So I wouldn't want the code at the very end; that would screw up that navigation mode.
msg207529 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-01-07 12:10
> I think maybe I only showed it to Stefan Krah, who said it wouldn't help his use case so I dropped it.

I think we were talking about _decimal, where any tool will interfere with the 100% code coverage patches.  But that's a special case.
msg207541 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 14:35
Patch attached.  I tweaked the punctuation in the last line, from this:

  /*[clinic end generated code; checksum: {checksum}]*/
                              ^         ^
to this:                      |         |
                              v         v
  /*[clinic end generated code: checksum={checksum}]*/

Guido, can you live with that?  I think it's a slight improvement but I'm not committed enough to fight about it.  I'll change it back if you want it your way.
msg207550 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-01-07 15:57
The new syntax is fine; I was only giving an example.
msg207591 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-07 20:43
I'm assuming this is sufficient.  If further bikeshedding is needed please reopen the issue.
msg207607 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-07 22:03
New changeset aa153113d273 by Zachary Ware in branch 'default':
Issue #19723: Fix issue number typo in Misc/NEWS
http://hg.python.org/cpython/rev/aa153113d273
msg207611 - (view) Author: Roundup Robot (python-dev) Date: 2014-01-07 22:25
New changeset e634b377d5cc by Larry Hastings in branch 'default':
Issue #19723: Missed one conversion to the new Argument Clinic syntax.
http://hg.python.org/cpython/rev/e634b377d5cc
History
Date User Action Args
2014-01-07 22:25:57python-devsetmessages: + msg207611
2014-01-07 22:03:29python-devsetnosy: + python-dev
messages: + msg207607
2014-01-07 20:43:42larrysetstatus: open -> closed
resolution: fixed
messages: + msg207591

stage: needs patch -> resolved
2014-01-07 15:57:26gvanrossumsetmessages: + msg207550
2014-01-07 14:35:50larrysetfiles: + larry.change.clinic.markers.diff.1.txt

messages: + msg207541
2014-01-07 12:10:17skrahsetmessages: + msg207529
2014-01-07 12:06:56larrysetmessages: + msg207528
2014-01-06 23:07:12larrysetmessages: + msg207493
2014-01-06 23:06:45gvanrossumsetmessages: + msg207492
2014-01-06 22:09:37serhiy.storchakasetmessages: + msg207491
2014-01-06 22:06:49serhiy.storchakasetmessages: + msg207490
2014-01-06 21:55:11gvanrossumsetmessages: + msg207488
2014-01-06 21:37:33pitrousetmessages: + msg207487
2014-01-06 21:35:50gvanrossumsetmessages: + msg207486
2014-01-06 21:19:07serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg207485
2014-01-06 21:01:36gvanrossumsetmessages: + msg207482
2014-01-06 21:00:13larrysetmessages: + msg207481
2014-01-06 20:56:21gvanrossumsetnosy: + gvanrossum
messages: + msg207480
2013-11-23 14:14:54ncoghlansetmessages: + msg204025
2013-11-22 22:05:44larrysetmessages: + msg203899
2013-11-22 20:52:21skrahsetnosy: + skrah
messages: + msg203869
2013-11-22 20:31:22pitrousetmessages: + msg203864
2013-11-22 20:27:26larrysetmessages: + msg203858
2013-11-22 20:04:04pitroucreate