issue_comments

@st-bender thank you very much! Great to have you as a contributor

Unit Test Results

4 files           4 suites   35m 31s :stopwatch: 16 210 tests 14 475 :heavy_check_mark: 1 735 :zzz: 0 :x: 60 304 runs  54 880 :heavy_check_mark: 5 424 :zzz: 0 :x:

Results for commit 4239b90a.

:recycle: This comment has been updated with latest results.

As for the tests, I found the tests that @max-sixty put in and extended them (see second and third commits in this PR). However, now there is one dataset setup and then 4(!) asserts, which seems to be too much to follow nicely. Imagine all of them break, you fix the first, only to find out that the second breaks as well. so you fix that, only to find out that the third breaks too, and so on.

I very much empathize! It was basically this frustration that drove me to writing that pytest-accept plugin. If you'd like to do these as doctests, that is fine too — whatever you think is best.

@keewis Thanks for the pointers, I'd say that nothing public facing should change in 0.18 now. OT (edit): By the way, these incompatibilities happen when one side decides to change the API without considering that some users may actually use that interface (and looking at the pandas' "deprecation" list, I fear that this will only get worse). Nice from the xarray people to have a section about keeping backwards compatibility as much as possible in their contribution guidelines.

As for the tests, I found the tests that @max-sixty put in and extended them (see second and third commits in this PR). However, now there is one dataset setup and then 4(!) asserts, which seems to be too much to follow nicely. Imagine all of them break, you fix the first, only to find out that the second breaks as well. so you fix that, only to find out that the third breaks too, and so on.

@Illviljan It is a good idea, however, I'd prefer if those changes were introduced as an option first, before changing the default behaviour.

I think this can wait until the next release. It would be nice if this and #5546 got released at the same time.

I also think we should be able to merge once the points @max-sixty mentioned have been addressed, but if you feel uncomfortable with merging now we can wait until after the next release (which should be soon, given that we have a compatibility issue with pandas=1.3).

The changelog is in /doc/whats-new.rst, and the reprs are documented in /doc/user-guide/data-structures.rst (not sure if that has to be modified), but there might be other files / docstrings.

For tests we usually use pytest.mark.parametrize. This would require a new test function (test_display_max_rows?), where you could check the number of lines of the repr.

Hi @max-sixty, Sure, but it will take a bit. Could you point me to right places for the docs? Just the filenames would do.

I would be in favour of waiting a little with this change to get a few more opinions. This change will ignore display_max_rows for everything except dataset.__repr__, which may not be what some people might expect or hope. Any ideas how to get more people to weigh in?

Edit: I would also like to separate the tests, to make it easier to follow if something breaks, but the setup for the test dataset would be the same. Any preferences or best practices for the code layout in such a case without duplicating too much of the code? However, that can probably wait for another PR.

This looks good! Thanks @st-bender .

Could we add a short note on the change to the whatsnew? To the extent the existing docs on the repr are now out of date, could we update those too?

Then we're good to merge!