gh-85052: Implement __repr__ for classes in csv module #125040
Conversation
dorosch
force-pushed
the
gh-85052-implement-repr-for-csv-module-classes
branch
from
00d444f to
87d086d
Compare
| return d | ||
|
|
||
| def __repr__(self): | ||
| return "%s('%s')" % (self.__class__.__name__, self.dialect) |
There was a problem hiding this comment.
Could we use an f-string instead? I don't see the need to use the ancient % operator:
| return "%s('%s')" % (self.__class__.__name__, self.dialect) | |
| return f"{self.__class__.__name__}(self.dialect!r})" |
(This goes for all the other cases as well.)
There was a problem hiding this comment.
I'm also not sure whether we should a fully-qualified name or not (namely, should we include the module's name or not?)
| } | ||
|
|
||
| static PyObject * | ||
| Dialect_str(DialectObj *self) { |
There was a problem hiding this comment.
PEP 7, and I'm not sure we need to take this as a DialectObj *. Since, as far as I can tell, we don't use any of the fields, we can use PyObject * instead to prevent a cast in the loop:
| Dialect_str(DialectObj *self) { | |
| Dialect_str(PyObject *self) | |
| { |
| Dialect_str(DialectObj *self) { | ||
| Py_ssize_t pos = 0; | ||
| PyObject *key, *value; | ||
| PyObject *module = PyType_GetModule(Py_TYPE(self)); |
There was a problem hiding this comment.
PyType_GetModule can fail, we need to check for NULL
| Py_INCREF(key); | ||
|
|
||
| return key; |
There was a problem hiding this comment.
Simplify:
| Py_INCREF(key); | |
| return key; | |
| return Py_NewRef(key); |
|
|
||
| static PyObject * | ||
| Dialect_repr(DialectObj *self) { | ||
| PyObject *str = Dialect_str(self); |
There was a problem hiding this comment.
This can also return NULL (PyUnicode_FromString can fail), so we need to check for that.
| } | ||
|
|
||
| static PyObject * | ||
| Dialect_repr(DialectObj *self) { |
There was a problem hiding this comment.
PEP 7 nitpick again
| Dialect_repr(DialectObj *self) { | |
| Dialect_repr(DialectObj *self) | |
| { |
|
Bénédikt, can you review the Python side of this? You (hopefully) have looked at |
| } | ||
|
|
||
| static PyObject * | ||
| Dialect_str(DialectObj *self) { |
| PyObject *dialects = get_csv_state(module)->dialects; | ||
|
|
||
| while (PyDict_Next(dialects, &pos, &key, &value)) { | ||
| if (value == (PyObject *)self) { |
There was a problem hiding this comment.
You can remove this when you change the signature.
| } | ||
| } | ||
|
|
||
| return PyUnicode_FromString("unknown"); |
There was a problem hiding this comment.
You could intern the string in the module's state.
| @@ -0,0 +1 @@ | |||
| Implement a ``__repr__`` method to :class:`csv.Dialect`, :class:`csv.DictReader` and :class:`csv.DictWriter`. | |||
There was a problem hiding this comment.
| Implement a ``__repr__`` method to :class:`csv.Dialect`, :class:`csv.DictReader` and :class:`csv.DictWriter`. | |
| Add :meth:`~object.__repr__` to :class:`csv.Dialect`, | |
| :class:`csv.DictReader` and :class:`csv.DictWriter`. |
| def test_dialect_repr(self): | ||
| for name in csv.list_dialects(): | ||
| dialect = csv.get_dialect(name) | ||
| self.assertRegex(repr(dialect), r"\w+\.Dialect\('%s'\)" % name) |
There was a problem hiding this comment.
| self.assertRegex(repr(dialect), r"\w+\.Dialect\('%s'\)" % name) | |
| self.assertRegex(repr(dialect), rf"\w+\.Dialect\({name!r}\)") |
There was a problem hiding this comment.
In addition, you could use with self.subTest(name) just to know which dialect is failing if there's any.
| return d | ||
|
|
||
| def __repr__(self): | ||
| return "%s('%s')" % (self.__class__.__name__, self.dialect) |
There was a problem hiding this comment.
I'm also not sure whether we should a fully-qualified name or not (namely, should we include the module's name or not?)
| writer = csv.DictWriter(fileobj, fieldnames=[]) | ||
| expected = "%s('%s')" % (writer.__class__.__name__, writer.dialect) | ||
| self.assertEqual(repr(writer), expected) | ||
|
|
| for name in csv.list_dialects(): | ||
| dialect = csv.get_dialect(name) | ||
| self.assertRegex(repr(dialect), r"\w+\.Dialect\('%s'\)" % name) | ||
|
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.