Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up Clinic's type expressions of buffers #68123

Closed
larryhastings opened this issue Apr 13, 2015 · 11 comments
Closed

Clean up Clinic's type expressions of buffers #68123

larryhastings opened this issue Apr 13, 2015 · 11 comments
Assignees
Labels
topic-argument-clinic type-bug An unexpected behavior, bug, or error

Comments

@larryhastings
Copy link
Contributor

BPO 23935
Nosy @larryhastings, @zware, @serhiy-storchaka, @pdmccormick
Files
  • larry.clinic.buffer.conceptual.cleanup.1.txt
  • larry.clinic.buffer.conceptual.cleanup.2.txt
  • larry.clinic.buffer.conceptual.cleanup.3.txt
  • larry.clinic.buffer.conceptual.cleanup.4.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 2015-04-16.03:03:37.271>
    created_at = <Date 2015-04-13.16:53:53.968>
    labels = ['type-bug', 'expert-argument-clinic']
    title = "Clean up Clinic's type expressions of buffers"
    updated_at = <Date 2015-04-16.03:03:37.270>
    user = 'https://github.com/larryhastings'

    bugs.python.org fields:

    activity = <Date 2015-04-16.03:03:37.270>
    actor = 'larry'
    assignee = 'larry'
    closed = True
    closed_date = <Date 2015-04-16.03:03:37.271>
    closer = 'larry'
    components = ['Argument Clinic']
    creation = <Date 2015-04-13.16:53:53.968>
    creator = 'larry'
    dependencies = []
    files = ['38933', '38948', '39001', '39030']
    hgrepos = []
    issue_num = 23935
    keywords = []
    message_count = 11.0
    messages = ['240660', '240661', '240720', '240861', '240975', '241030', '241078', '241080', '241087', '241193', '241194']
    nosy_count = 5.0
    nosy_names = ['larry', 'python-dev', 'zach.ware', 'serhiy.storchaka', 'pdmccormick']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23935'
    versions = ['Python 3.5']

    @larryhastings
    Copy link
    Contributor Author

    Clinic was previously pretty confused about what things accepted "bytes", "bytearray", "buffer", "robuffer", and "rwbuffer". This is because the documentation itself was somewhat confusing.

    The documentation was recently cleaned up on these in trunk (including one final fix from me this morning). This patch cleans up Clinic's understanding of how the types map to the format units.

    My notes on how the format units map to types is copy & pasted below.

    --

    the documentation for format units is a little confused, and it is therefore confusing

    inside:

    getbuffer (buffer)
    y* s* z*
    uses PyObject_GetBuffer(arg, view, PyBUF_SIMPLE)
    requires PyBuffer_IsContiguous(view, 'C')
    doesn't check specifically for bytes objects
    handles bytes objects
    accepts bytesarray

    convertbuffer (robuffer)
    y y# s# z#
    handles "read-only buffer object",
    all it really does is ensure it doesn't have a pb_releasebuffer,
    then call getbuffer() above

    so it doesn't accept bytesarray, *just because*

    doesn't accept bytes
    s z es es#

    requires writeable buffer (rwbuffer)
    w*

    actually checks / specifically handles bytes objects
    S

    actually checks / specifically handles bytearray objects
    Y

    actually checks / specifically handles bytes and bytearray objects
    c et et#

    @larryhastings larryhastings self-assigned this Apr 13, 2015
    @larryhastings larryhastings added topic-argument-clinic type-bug An unexpected behavior, bug, or error labels Apr 13, 2015
    @larryhastings
    Copy link
    Contributor Author

    Huh. Why didn't it attach my patch? Here it is.

    @larryhastings
    Copy link
    Contributor Author

    Updated the patch a little, to make it enforce compatible "types" parameters. The previous patch had some functions that would permit passing in "types='blah de blah'" or other nonsense.

    @serhiy-storchaka
    Copy link
    Member

    Added few comments on Rietveld.

    @larryhastings
    Copy link
    Contributor Author

    New patch revision, including the new API change (the "types" argument to a constructor must now be a set of strings).

    @serhiy-storchaka
    Copy link
    Member

    The new API change looks doubtful for me. It is more verbose. And less readable. It makes harder to search in sources, because {'str', 'robuffer'} is the same as {'robuffer', 'str'}. If Argument Clinic would a tool with programmatic API, sets would make sense, but for now the main interface of Argument Clinic is text inclusions in C files.

    If you want to convert the types parameter into a set, is it possible to continue to accept strings? Many user-friendly APIs accept either a sequence of string names or a string containing space-separated names. For example namedtuple, Enum.

    @larryhastings
    Copy link
    Contributor Author

    In the case of namedtuple and Enum, the parameter represents a sequence of strings--order is significant. With the 'types' parameter for converters, the internal model was always meant to be a *set* of strings. The order was explicitly *not* significant. So types="str robuffer" and types="robuffer str" should be identical. Your "harder to search in sources" was already true.

    Argument Clinic isn't something very many people will learn. Lots of people will read the conversions, but few will ever write a conversion, and they won't do it often and they'll forget the funny details. So we shouldn't make the API quirky and succinct at the expense of readability. The best API will be one that makes the code easier to read.

    I think requiring types to be a set of strings makes it easier to read because it makes the semantics obvious. If you see
    types="str robuffer"
    you aren't sure if order is significant, but if you see
    types={'str', 'robuffer'}
    you are certain it is not.

    @larryhastings
    Copy link
    Contributor Author

    Latest patch, with another round of lovely comments from Serhiy.

    @serhiy-storchaka
    Copy link
    Member

    Many people will see Argument Clinic syntax in declarations in C files. My recent patches (for _io, _ssl and _codecs modules) adds many types=. But I agree that there are many reasons to make this change.

    The patch LGTM. You can factorize it more if you like.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 16, 2015

    New changeset 4f74452a11e5 by Larry Hastings in branch 'default':
    Issue bpo-23935: Argument Clinic's understanding of format units
    https://hg.python.org/cpython/rev/4f74452a11e5

    @larryhastings
    Copy link
    Contributor Author

    While cleaning this up I noticed that es# and et# should both require "nullable". Fixed that too. I sure hope it's all correct now!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-argument-clinic type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants