TST: test custom _formatter for ExtensionArray + revert ExtensionArrayFormatter removal

[jorisvandenbossche]

xref #26833 (comment)

@simonjayhawkins this would be a test for the _formatter functionality (it is passing on master before the removal of ExtensionArrayFormatter

Adds an actual test for EA._formatter + reverts #26833 until we have a more complete solution for simplifying the formatters that take the EA._formatter into account.

[codecov]

Codecov Report

Merging #26845 into master will decrease coverage by 50.73%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #26845       +/-   ##
===========================================
- Coverage   91.86%   41.12%   -50.74%     
===========================================
  Files         179      179               
  Lines       50700    50700               
===========================================
- Hits        46576    20851    -25725     
- Misses       4124    29849    +25725

Flag	Coverage Δ
#multiple	?
#single	41.12% <ø> (-0.06%)	⬇️

Impacted Files	Coverage Δ
pandas/io/formats/latex.py	0% <0%> (-100%)	⬇️
pandas/plotting/_matplotlib/__init__.py	0% <0%> (-100%)	⬇️
pandas/io/gcs.py	0% <0%> (-100%)	⬇️
pandas/io/sas/sas_constants.py	0% <0%> (-100%)	⬇️
pandas/core/groupby/categorical.py	0% <0%> (-100%)	⬇️
pandas/tseries/plotting.py	0% <0%> (-100%)	⬇️
pandas/io/s3.py	0% <0%> (-100%)	⬇️
pandas/io/formats/html.py	0% <0%> (-99.37%)	⬇️
pandas/io/sas/sas7bdat.py	0% <0%> (-91.16%)	⬇️
pandas/io/sas/sas_xport.py	0% <0%> (-90.1%)	⬇️
... and 132 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8374a1d...c982520. Read the comment docs.

[codecov]

Codecov Report

Merging #26845 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26845      +/-   ##
==========================================
- Coverage   91.86%   91.86%   -0.01%     
==========================================
  Files         179      179              
  Lines       50700    50706       +6     
==========================================
+ Hits        46576    46580       +4     
- Misses       4124     4126       +2

Flag	Coverage Δ
#multiple	90.45% <100%> (ø)	⬆️
#single	41.1% <25%> (-0.08%)	⬇️

Impacted Files	Coverage Δ
pandas/io/formats/format.py	97.9% <100%> (+0.01%)	⬆️
pandas/core/indexes/interval.py	96.44% <100%> (ø)	⬆️
pandas/io/gbq.py	88.88% <0%> (-11.12%)	⬇️
pandas/io/formats/printing.py	85.56% <0%> (-0.54%)	⬇️
pandas/core/frame.py	96.88% <0%> (-0.12%)	⬇️
pandas/util/testing.py	90.84% <0%> (-0.11%)	⬇️
pandas/core/arrays/sparse.py	93.71% <0%> (+0.11%)	⬆️
pandas/core/arrays/categorical.py	95.92% <0%> (+0.12%)	⬆️
pandas/core/arrays/period.py	98.54% <0%> (+0.24%)	⬆️
pandas/core/arrays/base.py	99.43% <0%> (+0.56%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8374a1d...c982520. Read the comment docs.

[jreback]

why revert this? it is just causing churn. it was clear that the change was just a step in fixing this. tests are ok.

[jreback]

see comments

[jorisvandenbossche]

Did you read my comments in the PR? (#26833 (comment)) Then please give reasons why you would rather break the EA interface instead of temporarily reverting. We can fix issues with formatting in a better way in follow-up PRs.

Without the revert, the test will fail, so I could only add a xfailed test.

[jreback]

Did you read my comments in the PR? (#26833 (comment)) Then please give reasons why you would rather break the EA interface instead of temporarily reverting. We can fix issues with formatting in a better way in follow-up PRs.

Without the revert, the test will fail, so I could only add a xfailed test.

I read your comment. I disagree.

[TomAugspurger]

I’d prefer not breaking things.

…

On Jun 14, 2019, at 07:32, Jeff Reback ***@***.***> wrote: Did you read my comments in the PR? (#26833 (comment)) Then please give reasons why you would rather break the EA interface instead of temporarily reverting. We can fix issues with formatting in a better way in follow-up PRs. Without the revert, the test will fail, so I could only add a xfailed test. I read your comment. I disagree. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[jorisvandenbossche]

I read your comment. I disagree.

Can you then please give some reasoning?

[jreback]

how is this breaking an existing test? it did NOT. If you add something, then let's fix it. But reverting is kind of silly here. What to preserve some kind of history? this is just churning for no good reason.

It was very explicit that this was merged to provide an easier change path. I don't see anything that has changed.

If you want to merge an xfail test, sure.

[jorisvandenbossche]

Jeff, it only broke no tests because we forgot to properly test it. But what I am adding a test for here, has always been the intention of the _formatter method of the EA interface. And we for sure broke that.

[jreback]

Jeff, it only broke no tests because we forgot to properly test it. But what I am adding a test for here, has always been the intention of the _formatter method of the EA interface. And we for sure broke that.

EA._formatter is completely and utterly broken. Papering this over is not going to help. Let's just move forward. As I said I am happy to merge the test with an xfail.

[TomAugspurger]

Lets no break other libraries relying on the documented behavior.

…

On Jun 14, 2019, at 07:43, Jeff Reback ***@***.***> wrote: Jeff, it only broke no tests because we forgot to properly test it. But what I am adding a test for here, has always been the intention of the _formatter method of the EA interface. And we for sure broke that. EA._formatter is completely and utterly broken. Papering this over is not going to help. Let's just move forward. As I said I am happy to merge the test with an xfail. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[jorisvandenbossche]

EA._formatter is completely and utterly broken.

This method was purposefully introduced recently as part of the EA interface: #23601 (a PR that you approved, without calling it "utterly broken")

[jreback]

Lets no break other libraries relying on the documented behavior.

and w/o a test that is useless. If you want to fix it go right ahead. But as I said reverting is just churn. Would rather actually fix this, which has been broken for forever.

So this will be broken until then

[jreback]

This method was purposefully introduced recently as part of the EA interface: #23601 (a PR that you approved, without calling it "utterly broken")

That's utterly ridiculous, I approve virtually PRs.

[jreback]

Actually IIRC I voiced many many arguments on how Index / EA repr's are completely different and you guys pushed it thru.

So if its broken , then please fix it.

merging to avoid this vacuous argument.

[jorisvandenbossche]

Thanks for merging.

IIRC I voiced many many arguments on how Index / EA repr's are completely different and you guys pushed it thru.

Yes, we had disagreements about that. But as a final thing, I want to note that this discussion is not about the difference between Index vs Series vs EA reprs, but about the simple fact that EAs need a way to control how their scalar values are represented in any of those reprs. That is a basic functionality we had from the beginning of the EA interface (even before we had the discussion about the default repr).