[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by gruszczy
Modified:
1 year, 1 month ago
Reviewers:
pcmanticore
CC:
Georg, gruszczy, Claudiu.Popa, devnull_psf.upfronthosting.co.za, Petri Lehtinen
Visibility:
Public.

Patch Set 1 #

Total comments: 4

Patch Set 2 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/test_xdrlib.py View 1 1 chunk +24 lines, -0 lines 0 comments Download
Lib/xdrlib.py View 1 4 chunks +25 lines, -8 lines 0 comments Download

Messages

Total messages: 1
Expand All Messages | Collapse All Messages
Claudiu.Popa
1 year, 2 months ago #1
http://bugs.python.org/review/11694/diff/3337/Lib/xdrlib.py
File Lib/xdrlib.py (right):

http://bugs.python.org/review/11694/diff/3337/Lib/xdrlib.py#newcode34
Lib/xdrlib.py:34: def raising_conversion_error(function):
Please use @functools.wraps here, in order to maintain proper information about
the wrapped function.

http://bugs.python.org/review/11694/diff/3337/Lib/xdrlib.py#newcode34
Lib/xdrlib.py:34: def raising_conversion_error(function):
The name would be better if it used current tense, ``raise_conversion_errror``.

http://bugs.python.org/review/11694/diff/3337/Lib/xdrlib.py#newcode39
Lib/xdrlib.py:39: raise ConversionError(e)
You could use raise ConversionError(e) from None, I don't think that the
additional traceback is very useful.

http://bugs.python.org/review/11694/diff/3337/Lib/xdrlib.py#newcode74
Lib/xdrlib.py:74: self.pack_uint(x & 0xffffffff)
It's better to wrap only one call for which you except to raise an exception at
some point. Here, it's not clear that the first or the second call raises the
error and this imposes some difficulties in understand what code does and when.
Sign in to reply to this message.
Expand All Messages | Collapse All Messages