xdrlib raises ConversionError in inconsistent way

[gruszczy]

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]

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).

[akheron]

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]

Patch with tests.

[gruszczy]

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

[birkenfeld]

Looks good to me.

[akheron]

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]

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]

LGTM

[python-dev]

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]

Applied, thanks!