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
Comments
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). |
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. |
Patch with tests. |
Bump! It's almost 3 months since I posted the patch, so I would like to remind about this bug. |
Looks good to me. |
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: 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. |
Here's a refreshed patch:
|
LGTM |
New changeset 9cee201388c9 by Petri Lehtinen in branch '2.7': New changeset 7ef6e5f53418 by Petri Lehtinen in branch '3.4': New changeset 8e3387f566f6 by Petri Lehtinen in branch 'default': |
Applied, thanks! |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: