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

xdrlib raises ConversionError in inconsistent way #55903

Closed
gruszczy mannequin opened this issue Mar 27, 2011 · 10 comments
Closed

xdrlib raises ConversionError in inconsistent way #55903

gruszczy mannequin opened this issue Mar 27, 2011 · 10 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@gruszczy
Copy link
Mannequin

gruszczy mannequin commented Mar 27, 2011

BPO 11694
Nosy @birkenfeld, @PCManticore, @akheron
Files
  • 11694.patch
  • issue11694.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 2014-10-10.18:33:54.928>
    created_at = <Date 2011-03-27.11:12:16.299>
    labels = ['type-bug', 'library']
    title = 'xdrlib raises ConversionError in inconsistent way'
    updated_at = <Date 2014-10-10.18:33:54.926>
    user = 'https://bugs.python.org/gruszczy'

    bugs.python.org fields:

    activity = <Date 2014-10-10.18:33:54.926>
    actor = 'petri.lehtinen'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-10-10.18:33:54.928>
    closer = 'petri.lehtinen'
    components = ['Library (Lib)']
    creation = <Date 2011-03-27.11:12:16.299>
    creator = 'gruszczy'
    dependencies = []
    files = ['23178', '36854']
    hgrepos = []
    issue_num = 11694
    keywords = ['patch']
    message_count = 10.0
    messages = ['132309', '138783', '144180', '151291', '151293', '162367', '228893', '228951', '229020', '229021']
    nosy_count = 5.0
    nosy_names = ['georg.brandl', 'gruszczy', 'Claudiu.Popa', 'python-dev', 'petri.lehtinen']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue11694'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @gruszczy
    Copy link
    Mannequin Author

    gruszczy mannequin commented Mar 27, 2011

    xdrlib defines ConversionError, but very seldom uses it. For example:

        def pack_float(self, x):
            try: self.__buf.write(struct.pack('>f', x))
            except struct.error as msg:
                raise ConversionError(msg)

    But it doesn't do so here:

        def pack_uint(self, x):
            self.__buf.write(struct.pack('>L', x))

    Shouldn't that be more consistent?

    I am happy to write a patch, that will make xdrlib raise ConversionError, as well as write proper test (I believe xdrlib tests should get some love altogether, so I would add a separate test case for this).

    @gruszczy gruszczy mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 27, 2011
    @akheron
    Copy link
    Member

    akheron commented Jun 21, 2011

    This seems like a bug worth fixing. The ConversionError exception has been documented, an there's an example in the docs that suggest that at least all packing fails with a ConversionError.

    @gruszczy
    Copy link
    Mannequin Author

    gruszczy mannequin commented Sep 17, 2011

    Patch with tests.

    @gruszczy
    Copy link
    Mannequin Author

    gruszczy mannequin commented Jan 15, 2012

    Bump! It's almost 3 months since I posted the patch, so I would like to remind about this bug.

    @birkenfeld
    Copy link
    Member

    Looks good to me.

    @akheron
    Copy link
    Member

    akheron commented Jun 5, 2012

    I see one obvious issue with the patch: The ConversionErrors it creates are passed the struct.error or TypeError instance as a parameter. The first argument of these exceptions would be better, i.e.

    try:
    ...
    except struct.error as e:
    raise ConversionError(e.args[0])

    Furthermore, my ear thinks that raises_conversion_error would be a better name for the decorator than raising_conversion_error.

    Anyway, I think the whole ConversionError is a bit problematic, as either TypeError or ValueError would be the most appropriate exception, depending on the situation. For example:

    p = Packer()
    p.pack_int('foo')  # should raise a TypeError
    p.pack_int(2**100) # should raise a ValueError

    This would be slightly harder to implement, though, as struct.error has exactly the same problem.

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Oct 9, 2014

    Here's a refreshed patch:

    • raising_conversion_error is now raise_conversion_error
    • the decorator uses functools.wraps
    • the ConversionError is instantiated with the first argument of the caught expression
    • the reraising of ConversionError has the exception context supressed.

    @akheron
    Copy link
    Member

    akheron commented Oct 10, 2014

    LGTM

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 10, 2014

    New changeset 9cee201388c9 by Petri Lehtinen in branch '2.7':
    Issue bpo-11694: Raise ConversionError in xdrlib as documented
    https://hg.python.org/cpython/rev/9cee201388c9

    New changeset 7ef6e5f53418 by Petri Lehtinen in branch '3.4':
    Issue bpo-11694: Raise ConversionError in xdrlib as documented
    https://hg.python.org/cpython/rev/7ef6e5f53418

    New changeset 8e3387f566f6 by Petri Lehtinen in branch 'default':
    bpo-11694: merge with 3.4
    https://hg.python.org/cpython/rev/8e3387f566f6

    @akheron
    Copy link
    Member

    akheron commented Oct 10, 2014

    Applied, thanks!

    @akheron akheron closed this as completed Oct 10, 2014
    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants