Roundup Robot added the comment:
New changeset 0daf7f02c97f by Victor Stinner in branch '3.3':
Issue #18829: Add tests for the csv module for invalid characters (delimiter,
http://hg.python.org/cpython/rev/0daf7f02c97f
New changeset ccb52323039f by Victor Stinner in branch 'default':
(Merge
Changes by Serhiy Storchaka storch...@gmail.com:
--
assignee: - serhiy.storchaka
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18829
___
___
Roundup Robot added the comment:
New changeset 5ed75e36be8e by Serhiy Storchaka in branch '2.7':
Issue #18829: csv.Dialect() now checks type for delimiter, escapechar and
http://hg.python.org/cpython/rev/5ed75e36be8e
New changeset 52d03fbdf67a by Serhiy Storchaka in branch '3.3':
Issue #18829:
Serhiy Storchaka added the comment:
Thank you Vajrasky for your patch. I have simplified and fixed (escapechar can
be empty) it. Reverted ValueError back to TypeError because ord() raises
TypeError for non-1-character strings.
--
___
Python tracker
Changes by Serhiy Storchaka storch...@gmail.com:
--
resolution: - fixed
stage: patch review - committed/rejected
status: open - closed
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18829
Vajrasky Kok added the comment:
Well, what about None?
$ python3 -c 'import csv; reader = csv.reader(foo, delimiter=None)'
Traceback (most recent call last):
File string, line 1, in module
TypeError: delimiter must be set
English grammatically speaking, we should get this kind of error:
R. David Murray added the comment:
delimiter must be a 1 character string would cover it.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18829
___
Vajrasky Kok added the comment:
David R. Murray said, 'delimiter must be a 1 character string would cover it.'
You mean
$ ./python -c 'import csv; reader = csv.reader(foo, delimiter=)'
should give this error 'delimiter must be a 1 character string'?
Attached the patch to accommodate your
Vajrasky Kok added the comment:
Sorry for typing your name wrongly.
s/David R. Murray/R. David Murray/
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18829
___
Vajrasky Kok added the comment:
After contemplating for a while, I am not sure that we should treat empty
string for delimiter with error message: 'delimiter must be an 1-character
string'.
The reason is to keep consistency with quotechar keyword.
[sky@localhost cpython]$ ./python -c 'import
R. David Murray added the comment:
Parsing a csv file with no delimiter would seem to be meaningless, unless I'm
misunderstanding what 'delimeter' controls. So the error messages for
delimiter and quotechar are necessarily different.
--
___
Python
Vajrasky Kok added the comment:
I noticed in CPython source code, when we print the type of the object, we use
%.200s not %s. For example, in Modules/posixmodule.c:
uid should be integer, not %.200s,
So the newest path was created to conform with this tradition.
Another thing I noticed,
Serhiy Storchaka added the comment:
When specify an empty string error message is confusing too:
$ ./python -c 'import csv; reader = csv.reader(foo, delimiter=)'
Traceback (most recent call last):
File string, line 1, in module
TypeError: delimiter must be set
--
assignee:
Vajrasky Kok added the comment:
Attached the patch to accomodate Ezio Melotti's request. The patch now have two
separate errors for wrong type and right type, wrong length.
Don't we break compatibility by changing the type of exception from TypeError
to ValueError?
--
Added file:
Serhiy Storchaka added the comment:
A comment in csv.py says about compatibility with 2.3! After adding ValueError
to the list of catched exceptions we can keep this compatibility for future
generations. Here is a patch. I also have resorted the tests a little.
--
Added file:
Vajrasky Kok added the comment:
Same as fix_error_message_reader_csv_alternative_1_v4.patch (by Serhiy), but I
removed unused variables in the test.
--
Added file:
http://bugs.python.org/file31614/fix_error_message_reader_csv_alternative_1_v5.patch
Serhiy Storchaka added the comment:
I rather prefer adding new tests.
--
Added file:
http://bugs.python.org/file31615/fix_error_message_reader_csv_alternative_1_v6.patch
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18829
Ezio Melotti added the comment:
IMHO the error should say TypeError: delimiter must be an 1-character
string, not bytes.
We could also have two separate errors for wrong type and right type, wrong
length (this would be a ValueError though).
--
nosy: +ezio.melotti
Vajrasky Kok added the comment:
Apparently, other attributes of the csv dialect beside delimiter, such as
escapechar and quotechar share the same problem.
import _csv
_csv.reader('foo', quotechar=b'')
Traceback (most recent call last):
File stdin, line 1, in module
TypeError: quotechar
R. David Murray added the comment:
I agree that the 2.7 message is somewhat confusing, but I'm not sure it is
worth changing at this point in 2.7's life cycle.
For Python3, the message is correct and unambiguous: a bytes object is not a
string.
However, in 3.3, we have a regression:
R. David Murray added the comment:
I forgot to address the comment about accepting bytes in python3: the delimiter
really is a unicode character. In python3, non-ASCII delimiters are handled
correctly. So no, it isn't a byte anymore, it really is a string. Terry's
comment about 'char' in
Vajrasky Kok added the comment:
This is the preliminary patch to fix the error message.
--
keywords: +patch
nosy: +vajrasky
Added file: http://bugs.python.org/file31462/fix_error_message_reader_csv.patch
___
Python tracker rep...@bugs.python.org
Vajrasky Kok added the comment:
This is the second alternative of the patch to fix the error message using
different approach. I am not sure which one is better.
--
Added file:
http://bugs.python.org/file31463/fix_error_message_reader_csv_alternative_1.patch
Changes by Serhiy Storchaka storch...@gmail.com:
--
assignee: - serhiy.storchaka
nosy: +serhiy.storchaka
stage: needs patch - patch review
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue18829
Vajrasky Kok added the comment:
I realized there was a test that was similar with my test. I merged them into
one test.
I think the alternative is better than the original patch so I updated the
alternative patch only.
--
Added file:
25 matches
Mail list logo
