CLN/TST: delegate StringArray.fillna() to parent class + add tests

[arw2019]

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[jbrockmendel]

np.str_?

its a weird usage, but there's no reason why a user couldn't pass a n NA fill value.

[arw2019]

added both, including the missing value

[jorisvandenbossche]

np.str_ is already be covered by str (it subclasses str):

In [13]: arr = np.array(["a"])

In [14]: type(arr[0])
Out[14]: numpy.str_

In [15]: isinstance(arr[0], str)
Out[15]: True

[arw2019]

Ah right. Got rid of the np.str_ check

[arw2019]

Does this need a whatsnew?

[jorisvandenbossche]

Does this need a whatsnew?

I don't think it is necessarily needed, it's only a change in the error message I think?

[jreback]

cc @jbrockmendel

[jreback]

you should just be able to call _validate_setitem_value(value), maybe we can it it in the baseclass.

[jbrockmendel]

i would like this since it would allow us to implement a correct ExtensionBlock._can_hold_element, xref #36226

[arw2019]

you should just be able to call _validate_setitem_value(value), maybe we can it it in the baseclass.

Gave that a go - is the last commit what you meant? @jreback @jbrockmendel

[jorisvandenbossche]

I am -1 adding it in this PR (or at least adding it on the base class). If we add it to the base class, it needs to be part of the EA interface, documented that way, have a base extension test, have a fall back implementation, .. (and first discuss if we actually want it).

Also, we actually don't need to explicitly call the setitem validation here, since fillna is already raising that exception because it uses the setitem implementation under the hood.

So we could also simply add the test to ensure fillna raises the proper error.

[arw2019]

I am -1 adding it in this PR (or at least adding it on the base class). If we add it to the base class, it needs to be part of the EA interface, documented that way, have a base extension test, have a fall back implementation, .. (and first discuss if we actually want it).

Ok! I'll open an issue

Also, we actually don't need to explicitly call the setitem validation here, since fillna is already raising that exception because it uses the setitem implementation under the hood.

So we could also simply add the test to ensure fillna raises the proper error.

There was an existing test and I added more testcases here

[jorisvandenbossche]

This is actually not needed, as the setitem under the hood of fillna already calls it. So calling it here as well means we validate the value twice.

[arw2019]

right, reverted all the changes here

[arw2019]

I pared down this PR to just the fillna test for strings. Discussion on _validate_setitem_value is housed in #36226 (happy to work on that in a follow-on unless somebody else wants to)

[jbrockmendel]

does this method need to be overriden here at all?

[arw2019]

it doesn't

[arw2019]

pushed that now

[jreback]

thanks @arw2019