(#682) Fix row_counter behavior when mixing access methods of BoundRows.

[benmehlman]

Here's my fix for issue #682 which I reported yesterday.

First, I moved the row count() generator from Table to BoundRows.

Second, mirroring the existing relationship of BoundRows to Table, I added _boundrows as a member of BoundRow so that each BoundRow has knowledge of the BoundRows instance it came from.

Third, I pass the index of the first row in each BoundRows, or the only row in each BoundRow, so that the count always starts with the index of the first record in the BoundRows + 1.

The end result is that the row_counter is now tied to the index of the data row and will match regardless of the method by which you access it, even if you have multiple overlapping BoundRows instances.

Thanks!
Ben

[jieter]

Please ignore the notifications of me trying to fix the base of this PR, which seems to be resolved now.

With this patch, a couple of tests fail. I think the main difference here is that the second time a table instance is rendered, the row_counter starts at the last value of the the previous render. This is not desirable: rendering a table should be deterministic.

I also wonder why BoundRow.__init__ has a default for the boundrows param and what -1 means for the index param.

[benmehlman]

Re boundrows=None, probably because I was trying to avoid breaking other people's code in case someone, somewhere, instantiated a BoundRow using the current signature. But I can take that out.

And looking at the code now for index, I see that again, I probably did not want to intrude on the code more than necessary. The default of -1 results in a row number of 0 where index isn't specified. But I also see that it's necessary for me to touch the pinned row code for this to work. I don't use pinned rows in my tables... Let me take another pass through it and sort this out.

[benmehlman]

Hi, please don't commit to this branch. I've noticed some problems with it (as you pointed out.. it does not pass all the tests) and I am working on them and will let you know when it is ready to merge.. but in the meantime, my production environment is running on this branch! When you merged master you broke my production environment because all my code uses . as the accessor separator.. and I have DOZENS of tables in my applications, so the switch to __ breaks basically everything for me.

[jieter]

I'm sorry for the inconvenience I caused, I suspected it was a simple fix just before merging.

[benmehlman]

I'm sorry I left this hanging so long, things got super busy here.. then the pandemic. I'm now looking at these patches and wondering whether they are still relevant. If so I'll try to resolve the issue with pinned rows. We here are currently still on django 1.11 (python 3) so we are running on my fork of tables2 all this time. I would like to update to the latest if this row count issue can be resolved. Thanks!