-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-85052: Implement __repr__ for classes in csv module #125040
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -204,6 +204,7 @@ class BadItem: | |||||||
| def __str__(self): | ||||||||
| raise OSError | ||||||||
| self._write_error_test(OSError, [BadItem()]) | ||||||||
|
|
||||||||
| def test_write_bigfield(self): | ||||||||
| # This exercises the buffer realloc functionality | ||||||||
| bigstring = 'X' * 50000 | ||||||||
|
|
@@ -551,6 +552,19 @@ def test_roundtrip_escaped_unquoted_newlines(self): | |||||||
| escapechar="\\")): | ||||||||
| self.assertEqual(row, rows[i]) | ||||||||
|
|
||||||||
| def test_dict_reader_repr(self): | ||||||||
| fileobj = StringIO() | ||||||||
| reader = csv.DictReader(fileobj) | ||||||||
| expected = "%s('%s')" % (reader.__class__.__name__, reader.dialect) | ||||||||
| self.assertEqual(repr(reader), expected) | ||||||||
|
|
||||||||
| def test_dict_writer_repr(self): | ||||||||
| fileobj = StringIO() | ||||||||
| writer = csv.DictWriter(fileobj, fieldnames=[]) | ||||||||
| expected = "%s('%s')" % (writer.__class__.__name__, writer.dialect) | ||||||||
| self.assertEqual(repr(writer), expected) | ||||||||
|
|
||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
|
|
||||||||
|
|
||||||||
| class TestDialectRegistry(unittest.TestCase): | ||||||||
| def test_registry_badargs(self): | ||||||||
|
|
@@ -685,6 +699,16 @@ def test_pickle(self): | |||||||
| for proto in range(pickle.HIGHEST_PROTOCOL + 1): | ||||||||
| self.assertRaises(TypeError, pickle.dumps, dialect, proto) | ||||||||
|
|
||||||||
| def test_dialect_str(self): | ||||||||
| for name in csv.list_dialects(): | ||||||||
| dialect = csv.get_dialect(name) | ||||||||
| self.assertEqual(str(dialect), name) | ||||||||
|
|
||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition, you could use |
||||||||
|
|
||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| class TestCsvBase(unittest.TestCase): | ||||||||
| def readerAssertEqual(self, input, expected_result): | ||||||||
| with TemporaryFile("w+", encoding="utf-8", newline='') as fileobj: | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -603,6 +603,34 @@ Dialect_traverse(DialectObj *self, visitproc visit, void *arg) | |||||||||
| return 0; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| static PyObject * | ||||||||||
| Dialect_str(DialectObj *self) { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PEP 7, and I'm not sure we need to take this as a
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also fixes any future UBsan |
||||||||||
| Py_ssize_t pos = 0; | ||||||||||
| PyObject *key, *value; | ||||||||||
| PyObject *module = PyType_GetModule(Py_TYPE(self)); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove this when you change the signature. |
||||||||||
| Py_INCREF(key); | ||||||||||
|
|
||||||||||
| return key; | ||||||||||
|
Comment on lines
+615
to
+617
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplify:
Suggested change
|
||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return PyUnicode_FromString("unknown"); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could intern the string in the module's state. |
||||||||||
| } | ||||||||||
|
|
||||||||||
| static PyObject * | ||||||||||
| Dialect_repr(DialectObj *self) { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PEP 7 nitpick again
Suggested change
|
||||||||||
| PyObject *str = Dialect_str(self); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can also return |
||||||||||
| PyObject *repr = PyUnicode_FromFormat("%s(%R)", Py_TYPE(self)->tp_name, str); | ||||||||||
|
|
||||||||||
| Py_DECREF(str); | ||||||||||
|
|
||||||||||
| return repr; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| static PyType_Slot Dialect_Type_slots[] = { | ||||||||||
| {Py_tp_doc, (char*)Dialect_Type_doc}, | ||||||||||
| {Py_tp_members, Dialect_memberlist}, | ||||||||||
|
|
@@ -612,6 +640,8 @@ static PyType_Slot Dialect_Type_slots[] = { | |||||||||
| {Py_tp_dealloc, Dialect_dealloc}, | ||||||||||
| {Py_tp_clear, Dialect_clear}, | ||||||||||
| {Py_tp_traverse, Dialect_traverse}, | ||||||||||
| {Py_tp_str, Dialect_str}, | ||||||||||
| {Py_tp_repr, Dialect_repr}, | ||||||||||
| {0, NULL} | ||||||||||
| }; | ||||||||||
|
|
||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use an f-string instead? I don't see the need to use the ancient
%operator:(This goes for all the other cases as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure whether we should a fully-qualified name or not (namely, should we include the module's name or not?)