classification
Title: xdrlib raises ConversionError in inconsistent way
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Claudiu.Popa, georg.brandl, gruszczy, petri.lehtinen, python-dev
Priority: normal Keywords: patch

Created on 2011-03-27 11:12 by gruszczy, last changed 2014-10-10 18:33 by petri.lehtinen. This issue is now closed.

Files
File name Uploaded Description Edit
11694.patch gruszczy, 2011-09-17 10:59 review
issue11694.patch Claudiu.Popa, 2014-10-09 19:16 review
Messages (10)
msg132309 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2011-03-27 11:12
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).
msg138783 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-06-21 12:30
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.
msg144180 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2011-09-17 10:59
Patch with tests.
msg151291 - (view) Author: Filip Gruszczyński (gruszczy) Date: 2012-01-15 15:50
Bump! It's almost 3 months since I posted the patch, so I would like to remind about this bug.
msg151293 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-01-15 16:54
Looks good to me.
msg162367 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2012-06-05 19:24
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.
msg228893 - (view) Author: Claudiu Popa (Claudiu.Popa) * Date: 2014-10-09 19:16
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.
msg228951 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2014-10-10 05:23
LGTM
msg229020 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-10 18:30
New changeset 9cee201388c9 by Petri Lehtinen in branch '2.7':
Issue #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 #11694: Raise ConversionError in xdrlib as documented
https://hg.python.org/cpython/rev/7ef6e5f53418

New changeset 8e3387f566f6 by Petri Lehtinen in branch 'default':
#11694: merge with 3.4
https://hg.python.org/cpython/rev/8e3387f566f6
msg229021 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2014-10-10 18:33
Applied, thanks!
History
Date User Action Args
2014-10-10 18:33:54petri.lehtinensetstatus: open -> closed
resolution: fixed
messages: + msg229021

stage: patch review -> resolved
2014-10-10 18:30:41python-devsetnosy: + python-dev
messages: + msg229020
2014-10-10 05:23:31petri.lehtinensetmessages: + msg228951
2014-10-09 19:16:06Claudiu.Popasetfiles: + issue11694.patch

messages: + msg228893
versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2014-10-04 18:27:34Claudiu.Popasetnosy: + Claudiu.Popa
2012-06-05 19:24:15petri.lehtinensetmessages: + msg162367
2012-01-15 16:54:32georg.brandlsetstage: test needed -> patch review
2012-01-15 16:54:21georg.brandlsetnosy: + georg.brandl
messages: + msg151293
2012-01-15 15:50:39gruszczysetmessages: + msg151291
2011-09-17 10:59:26gruszczysetfiles: + 11694.patch
keywords: + patch
messages: + msg144180
2011-07-24 18:26:12petri.lehtinensetstage: test needed
versions: + Python 2.7, Python 3.2
2011-06-21 12:30:42petri.lehtinensetnosy: + petri.lehtinen
messages: + msg138783
2011-03-27 11:12:16gruszczycreate