Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Lib/csv.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,9 @@ def __next__(self):
d[key] = self.restval
return d

def __repr__(self):
return "%s('%s')" % (self.__class__.__name__, self.dialect)
Copy link
Member

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:

Suggested change
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.)

Copy link
Member

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?)


__class_getitem__ = classmethod(types.GenericAlias)


Expand All @@ -208,6 +211,7 @@ def __init__(self, f, fieldnames, restval="", extrasaction="raise",
raise ValueError("extrasaction (%s) must be 'raise' or 'ignore'"
% extrasaction)
self.extrasaction = extrasaction
self.dialect = dialect
self.writer = writer(f, dialect, *args, **kwds)

def writeheader(self):
Expand All @@ -228,6 +232,9 @@ def writerow(self, rowdict):
def writerows(self, rowdicts):
return self.writer.writerows(map(self._dict_to_list, rowdicts))

def __repr__(self):
return "%s('%s')" % (self.__class__.__name__, self.dialect)

__class_getitem__ = classmethod(types.GenericAlias)


Expand Down
24 changes: 24 additions & 0 deletions Lib/test/test_csv.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change



class TestDialectRegistry(unittest.TestCase):
def test_registry_badargs(self):
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.assertRegex(repr(dialect), r"\w+\.Dialect\('%s'\)" % name)
self.assertRegex(repr(dialect), rf"\w+\.Dialect\({name!r}\)")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, you could use with self.subTest(name) just to know which dialect is failing if there's any.


Copy link
Member

Choose a reason for hiding this comment

The 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:
Expand Down
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`.
Copy link
Member

@picnixz picnixz Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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`.

30 changes: 30 additions & 0 deletions Modules/_csv.c
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,34 @@ Dialect_traverse(DialectObj *self, visitproc visit, void *arg)
return 0;
}

static PyObject *
Dialect_str(DialectObj *self) {
Copy link
Member

Choose a reason for hiding this comment

The 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 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:

Suggested change
Dialect_str(DialectObj *self) {
Dialect_str(PyObject *self)
{

Copy link
Member

Choose a reason for hiding this comment

The 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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyType_GetModule can fail, we need to check for NULL

PyObject *dialects = get_csv_state(module)->dialects;

while (PyDict_Next(dialects, &pos, &key, &value)) {
if (value == (PyObject *)self) {
Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify:

Suggested change
Py_INCREF(key);
return key;
return Py_NewRef(key);

}
}

return PyUnicode_FromString("unknown");
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP 7 nitpick again

Suggested change
Dialect_repr(DialectObj *self) {
Dialect_repr(DialectObj *self)
{

PyObject *str = Dialect_str(self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also return NULL (PyUnicode_FromString can fail), so we need to check for that.

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},
Expand All @@ -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}
};

Expand Down
Loading