bpo-20028: Improve error message of csv.Dialect when initializing #28705

corona10 · 2021-10-03T12:15:55Z

https://bugs.python.org/issue20028

corona10 · 2021-10-03T12:18:04Z

./python.exe -m test test_csv -R 3:3
0:00:00 load avg: 2.50 Run tests sequentially
0:00:00 load avg: 2.50 [1/1] test_csv
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 3.6 sec
Tests result: SUCCESS

Modules/_csv.c

corona10 · 2021-10-06T00:39:56Z

@smontanaro Can you please take a look?

smontanaro · 2021-10-06T02:44:51Z

Modules/_csv.c

@@ -457,7 +465,7 @@ dialect_new(PyTypeObject *type, PyObject *args, PyObject *kwargs)
        goto err;
    if (self->delimiter == 0) {
        PyErr_SetString(PyExc_TypeError,
-                        "\"delimiter\" must be a 1-character string");
+                        "delimiter must be set");


I think "set" might confuse naive readers. What was wrong with the previous spelling? Also, note that "delimiter" was quoted because it was explicitly referring to the "dellimiter" attribute.

Okay, I agree that the message doesn't need to be changed.

It was made consistent with error messages for other arguments.

smontanaro · 2021-10-06T02:45:24Z

@iritkatriel While the new test ("!=" vs ">") is technically more correct and might be worthwhile in 3.11, I don't think that change can be backported, since it might break existing code.

corona10 · 2021-10-07T07:47:31Z

@iritkatriel gentle ping :)

iritkatriel · 2021-10-08T16:03:48Z

Looks fine, would be nice to add a coauthored-by crediting the original patch author.

corona10 · 2021-10-09T07:30:12Z

@serhiy-storchaka
if we apply #28808, should we drop this PR?
This PR will make an error .

self._read_test(['a,\\b,c'], [['a', '\\b', 'c']], escapechar='')
# TypeError: "escapechar" must be a 1-character string

serhiy-storchaka · 2021-10-09T10:06:40Z

#28808 and this PR are almost unrelyted. I was going to merge #28808 after merging this PR if the purpose of this PR is just improving error messages.

serhiy-storchaka · 2021-10-09T10:15:04Z

Modules/_csv.c

-                PyErr_Format(PyExc_TypeError,
-                    "\"%s\" must be string, not %.200s", name,
-                    Py_TYPE(src)->tp_name);
+                if (strcmp(name, "delimiter") == 0) {


If we tweak this error message, we should also raise the same error if delimiter=None.

It may be simpler to introduce different function _set_char_or_none().

corona10 · 2021-10-09T13:31:49Z

@serhiy-storchaka
Thanks for the review.

It may be simpler to introduce different function _set_char_or_none()

I follow your code review, Please take a look.

PR is just improving error messages.

This PR will not allow zero-length string, is this should be allowed?

self._read_test(['a,\0b,c'], [['a', 'b', 'c']], escapechar='\0') -> allowed
self._read_test(['a,\b,c'], [['a', '\b', 'c']], escapechar='') -> not allowed.

Please let me know your intention, I will update the PR to follow your intention :)

Modules/_csv.c

serhiy-storchaka · 2021-10-09T13:47:40Z

This PR will not allow zero-length string, is this should be allowed?

This is undocumented behavior, and I believe it is not intentional. I think that the change for more correct error message could be backported to 3.9 and 3.10, but the change in handling empty strings should be only in 3.11.

Also there is a bug, error in PyUnicode_GetLength() is not handled. Its fix should be backported too.

So you can split this PR on 2 or 3 PRs (better error message, handle error in PyUnicode_GetLength(), disallow empty escapechar/quotechar) and merge and backport them separately. or merge the combined PR and remove some changes in backports.

Ah, and it would be nice to disallow empty lineterminator, and disallow conflicts (e.g. the same char for delimiter, escapechar or quotechar), but this is a different issue.

corona10 · 2021-10-09T13:53:37Z

Okay let's separate the PR into 3 PRs

part1 better error message
part2 handle error in PyUnicode_GetLength()
part3 disallow empty escapechar/quotechar

miss-islington · 2021-10-09T14:50:15Z

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

bedevere-bot · 2021-10-09T14:50:25Z

GH-28830 is a backport of this pull request to the 3.10 branch.

bedevere-bot · 2021-10-09T14:50:29Z

GH-28831 is a backport of this pull request to the 3.9 branch.

…thonGH-28705) (cherry picked from commit 34bbc87) Co-authored-by: Dong-hee Na <donghee.na@python.org>

…-28705) (cherry picked from commit 34bbc87) Co-authored-by: Dong-hee Na <donghee.na@python.org>

bedevere-bot · 2021-10-09T16:53:40Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.9 has failed when building commit 6f3bc5e.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/527/builds/264) and take a look at the build logs.
Check if the failure is related to this commit (6f3bc5e) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/527/builds/264

Failed tests:

test_ttk_guionly
test_tk

Failed subtests:

test_configure_compound - tkinter.test.test_ttk.test_widgets.LabelTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.CheckbuttonTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.RadiobuttonTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.ButtonTest
test_configure_type - tkinter.test.test_tkinter.test_widgets.MenuTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.MenubuttonTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

410 tests OK.

2 tests failed:
test_tk test_ttk_guionly

13 tests skipped:
test_asdl_parser test_check_c_globals test_clinic test_devpoll
test_gdb test_ioctl test_kqueue test_msilib test_startfile
test_winconsoleio test_winreg test_winsound test_zipfile64

2 re-run tests:
test_tk test_ttk_guionly

Total duration: 39 min

Click to see traceback logs

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.installed/build/target/lib/python3.9/tkinter/test/test_tkinter/test_widgets.py", line 1244, in test_configure_type
    self.checkEnumParam(widget, 'type',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.installed/build/target/lib/python3.9/tkinter/test/widget_tests.py", line 151, in checkEnumParam
    self.checkInvalidParam(widget, name, '',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.installed/build/target/lib/python3.9/tkinter/test/widget_tests.py", line 74, in checkInvalidParam
    widget[name] = value
AssertionError: TclError not raised


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.installed/build/target/lib/python3.9/tkinter/test/test_ttk/test_widgets.py", line 173, in test_configure_compound
    self.checkEnumParam(widget, 'compound',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.installed/build/target/lib/python3.9/tkinter/test/widget_tests.py", line 151, in checkEnumParam
    self.checkInvalidParam(widget, name, '',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.installed/build/target/lib/python3.9/tkinter/test/widget_tests.py", line 74, in checkInvalidParam
    widget[name] = value
AssertionError: TclError not raised

bedevere-bot · 2021-10-09T17:23:10Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.9 has failed when building commit 6f3bc5e.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/51/builds/265) and take a look at the build logs.
Check if the failure is related to this commit (6f3bc5e) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/51/builds/265

Failed tests:

test_ttk_guionly
test_tk

Failed subtests:

test_configure_compound - tkinter.test.test_ttk.test_widgets.LabelTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.CheckbuttonTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.RadiobuttonTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.ButtonTest
test_configure_type - tkinter.test.test_tkinter.test_widgets.MenuTest
test_configure_compound - tkinter.test.test_ttk.test_widgets.MenubuttonTest

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

414 tests OK.

10 slowest tests:

test_peg_generator: 19 min 45 sec
test_concurrent_futures: 5 min 27 sec
test_unparse: 4 min 55 sec
test_tokenize: 4 min 44 sec
test_multiprocessing_spawn: 4 min 42 sec
test_lib2to3: 3 min 16 sec
test_asyncio: 2 min 51 sec
test_multiprocessing_forkserver: 2 min 50 sec
test_capi: 1 min 39 sec
test_multiprocessing_fork: 1 min 39 sec

2 tests failed:
test_tk test_ttk_guionly

9 tests skipped:
test_devpoll test_ioctl test_kqueue test_msilib test_startfile
test_winconsoleio test_winreg test_winsound test_zipfile64

2 re-run tests:
test_tk test_ttk_guionly

Total duration: 49 min 45 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.nondebug/build/Lib/tkinter/test/test_ttk/test_widgets.py", line 173, in test_configure_compound
    self.checkEnumParam(widget, 'compound',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.nondebug/build/Lib/tkinter/test/widget_tests.py", line 151, in checkEnumParam
    self.checkInvalidParam(widget, name, '',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.nondebug/build/Lib/tkinter/test/widget_tests.py", line 74, in checkInvalidParam
    widget[name] = value
AssertionError: TclError not raised


Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.nondebug/build/Lib/tkinter/test/test_tkinter/test_widgets.py", line 1244, in test_configure_type
    self.checkEnumParam(widget, 'type',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.nondebug/build/Lib/tkinter/test/widget_tests.py", line 151, in checkEnumParam
    self.checkInvalidParam(widget, name, '',
  File "/buildbot/buildarea/cpython/3.9.ware-gentoo-x86.nondebug/build/Lib/tkinter/test/widget_tests.py", line 74, in checkInvalidParam
    widget[name] = value
AssertionError: TclError not raised

corona10 requested a review from iritkatriel Oct 3, 2021

bedevere-bot added the awaiting core review label Oct 3, 2021

the-knights-who-say-ni added the CLA signed label Oct 3, 2021

corona10 added needs backport to 3.9 needs backport to 3.10 labels Oct 3, 2021

iritkatriel reviewed Oct 3, 2021

View changes

Modules/_csv.c Show resolved Hide resolved

corona10 requested a review from iritkatriel Oct 4, 2021

smontanaro reviewed Oct 6, 2021

View changes

corona10 removed needs backport to 3.9 needs backport to 3.10 labels Oct 6, 2021

iritkatriel approved these changes Oct 8, 2021

View changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 8, 2021

corona10 added 4 commits Oct 9, 2021

bpo-20028: Update csv module to hanlde the giving invalid quotechar.

ed9d558

bpo-20028: revert error message

c9f4941

bpo-20028: test refactoring

638f7d0

bpo-20028: Add authors

85a418d

corona10 force-pushed the bpo-20028 branch from 0bc90a9 to 85a418d Oct 9, 2021

serhiy-storchaka reviewed Oct 9, 2021

View changes

bpo-20028: Address code review

611fb4f

corona10 force-pushed the bpo-20028 branch from e96d6b0 to 611fb4f Oct 9, 2021

serhiy-storchaka reviewed Oct 9, 2021

View changes

Modules/_csv.c Outdated Show resolved Hide resolved

Modules/_csv.c Show resolved Hide resolved

bpo-20028: Address code review

a9fd17b

corona10 added needs backport to 3.9 needs backport to 3.10 labels Oct 9, 2021

bpo-20028: Seperate the PR

5680bff

corona10 force-pushed the bpo-20028 branch from 170adb7 to 5680bff Oct 9, 2021

corona10 changed the title ~~bpo-20028: Update csv module to hanlde the giving invalid quotechar.~~ bpo-20028: Improve error message of csv.Dialect when initializing Oct 9, 2021

corona10 merged commit 34bbc87 into python:main Oct 9, 2021
12 checks passed

bedevere-bot removed the awaiting merge label Oct 9, 2021

bedevere-bot removed needs backport to 3.10 needs backport to 3.9 labels Oct 9, 2021

miss-islington added a commit to miss-islington/cpython that referenced this issue Oct 9, 2021

bpo-20028: Improve error message of csv.Dialect when initializing (py…

5ee5ed2

…thonGH-28705) (cherry picked from commit 34bbc87) Co-authored-by: Dong-hee Na <donghee.na@python.org>

miss-islington added a commit to miss-islington/cpython that referenced this issue Oct 9, 2021

bpo-20028: Improve error message of csv.Dialect when initializing (py…

e3e6407

…thonGH-28705) (cherry picked from commit 34bbc87) Co-authored-by: Dong-hee Na <donghee.na@python.org>

corona10 deleted the bpo-20028 branch Oct 9, 2021

miss-islington added a commit that referenced this issue Oct 9, 2021

bpo-20028: Improve error message of csv.Dialect when initializing (GH…

6f3bc5e

…-28705) (cherry picked from commit 34bbc87) Co-authored-by: Dong-hee Na <donghee.na@python.org>

miss-islington added a commit that referenced this issue Oct 9, 2021

bpo-20028: Improve error message of csv.Dialect when initializing (GH…

8772935

…-28705) (cherry picked from commit 34bbc87) Co-authored-by: Dong-hee Na <donghee.na@python.org>

corona10 mentioned this pull request Oct 11, 2021

Handle error when PyUnicode_GetLength returns a negative value. #28859

Merged

python / cpython Public

bpo-20028: Improve error message of csv.Dialect when initializing #28705

bpo-20028: Improve error message of csv.Dialect when initializing #28705

corona10 commented Oct 3, 2021 •

edited by bedevere-bot

corona10 commented Oct 3, 2021

corona10 commented Oct 6, 2021

smontanaro Oct 6, 2021

corona10 Oct 6, 2021

serhiy-storchaka Oct 7, 2021

smontanaro commented Oct 6, 2021 •

edited

corona10 commented Oct 7, 2021

iritkatriel commented Oct 8, 2021

corona10 commented Oct 9, 2021 •

edited

serhiy-storchaka commented Oct 9, 2021

serhiy-storchaka Oct 9, 2021

corona10 commented Oct 9, 2021 •

edited

serhiy-storchaka commented Oct 9, 2021

corona10 commented Oct 9, 2021

miss-islington commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

python / cpython Public

bpo-20028: Improve error message of csv.Dialect when initializing #28705

bpo-20028: Improve error message of csv.Dialect when initializing #28705

Conversation

corona10 commented Oct 3, 2021 • edited by bedevere-bot

corona10 commented Oct 3, 2021

corona10 commented Oct 6, 2021

smontanaro Oct 6, 2021

corona10 Oct 6, 2021

serhiy-storchaka Oct 7, 2021

smontanaro commented Oct 6, 2021 • edited

corona10 commented Oct 7, 2021

iritkatriel commented Oct 8, 2021

corona10 commented Oct 9, 2021 • edited

serhiy-storchaka commented Oct 9, 2021

serhiy-storchaka Oct 9, 2021

corona10 commented Oct 9, 2021 • edited

serhiy-storchaka commented Oct 9, 2021

corona10 commented Oct 9, 2021

miss-islington commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

bedevere-bot commented Oct 9, 2021

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

bedevere-bot commented Oct 9, 2021

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

corona10 commented Oct 3, 2021 •

edited by bedevere-bot

smontanaro commented Oct 6, 2021 •

edited

corona10 commented Oct 9, 2021 •

edited

corona10 commented Oct 9, 2021 •

edited