BUG: read_csv raising ValueError for tru_values/false_values and boolean dtype

[phofl]

• closes read_csv cannot use dtype and true_values/false_values #34655
• tests added / passed
• Ensure all linting tests pass, see here for how to run them
• whatsnew entry

I am not really experienced with cython, so I would appreciate feedback on the switiching function. This was not done previously in case of ea boolean dtype, hence why this was failing before.

[jreback]

i would instead allow passing in the true/false values into BooleanArrary._from_sequence_from_strings (and if they aren't passed, then use what is there now). and handle the conversion there rather than doing it this way.

[phofl]

Hm this sounds good. Will try this tomorrow or Friday. Thx.

[phofl]

This is way better. To avoid special casing the boolean case, I added **kwargs to _from_sequence_of_strings, Is there another way you would prefer?

[jorisvandenbossche]

Passing such additional keywords to _from_sequence_of_strings is not backwards compatible for externally defined EAs, if they do not accepts kwargs.
So I would maybe keep a special case check for boolean dtype in _parsers.pyx, and only pass the keywords to BooleanArray._from_sequence_of_strings.

(alternative could be to try/except, catch the error, and then try again without passing the additional keywords (and potentially raising a warning so that external EAs can update to accept **kwargs)

[jreback]

Passing such additional keywords to _from_sequence_of_strings is not backwards compatible for externally defined EAs, if they do not accepts kwargs.
So I would maybe keep a special case check for boolean dtype in _parsers.pyx, and only pass the keywords to BooleanArray._from_sequence_of_strings.

(alternative could be to try/except, catch the error, and then try again without passing the additional keywords (and potentially raising a warning so that external EAs can update to accept **kwargs)

we don't have these kinds of guarantees on EA. I think we should accept them.

[jorisvandenbossche]

_from_sequence_of_strings is a public method of the ExtensionArray Interface, documented through means of the base class implementation:

pandas/pandas/core/arrays/base.py

Lines 213 to 237 in 510bc20

	@classmethod
	def _from_sequence_of_strings(
	cls, strings, *, dtype: Optional[Dtype] = None, copy=False
	):
	"""
	Construct a new ExtensionArray from a sequence of strings.
	.. versionadded:: 0.24.0
	Parameters
	----------
	strings : Sequence
	Each element will be an instance of the scalar type for this
	array, ``cls.dtype.type``.
	dtype : dtype, optional
	Construct for this particular dtype. This should be a Dtype
	compatible with the ExtensionArray.
	copy : bool, default False
	If True, copy the underlying data.
	Returns
	-------
	ExtensionArray
	"""
	raise AbstractMethodError(cls)

(and in the online API docs)

If we can easily avoid breaking third-party EA implementations, then we should do that.

[jreback]

_from_sequence_of_strings is a public method of the ExtensionArray Interface, documented through means of the base class implementation:

pandas/pandas/core/arrays/base.py

Lines 213 to 237 in 510bc20

	@classmethod
	def _from_sequence_of_strings(
	cls, strings, *, dtype: Optional[Dtype] = None, copy=False
	):
	"""
	Construct a new ExtensionArray from a sequence of strings.
	.. versionadded:: 0.24.0
	Parameters
	----------
	strings : Sequence
	Each element will be an instance of the scalar type for this
	array, ``cls.dtype.type``.
	dtype : dtype, optional
	Construct for this particular dtype. This should be a Dtype
	compatible with the ExtensionArray.
	copy : bool, default False
	If True, copy the underlying data.
	Returns
	-------
	ExtensionArray
	"""
	raise AbstractMethodError(cls)

(and in the online API docs)

If we can easily avoid breaking third-party EA implementations, then we should do that.

I think we need to in this case (for BooleanArray) to make a proper api. yes I agree we can avoid breaking all others (see above my comment).

[jorisvandenbossche]

Yep, no problem for adding it to BooleanArray, that's our own EA implementation, there we can add keyword however we want.

[jorisvandenbossche]

Thanks for the updates!

[jreback]

these could also be module level (doesn't matter), note in the parser itself we only have

    object _true_values = [b'True', b'TRUE', b'true']
    object _false_values = [b'False', b'FALSE', b'false']

but yeah i think we agreed the 1/1.0 are fine.

[phofl]

In this case let's keep it there

[jreback]

small comment

[jreback]

lgtm

[jreback]

thanks @phofl very nice!