Re: [lmi] Pagination anomaly

[Greg Chicares]

On 2018-02-06 14:21, Vadim Zeitlin wrote:
> On Tue, 6 Feb 2018 12:52:39 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> Then would it make sense to abstract out the pagination code,
> GC> and add a unit test like the one you posted yesterday?
> 
>  For get_extra_pages_needed() this could be done easily enough. But
> render() is not as simple to untangle... If we do decide to keep both
> versions of the code, then it would be worth doing it for just
> get_extra_pages_needed(), but if we're going to remove it anyhow, then it
> wouldn't seem to make much sense.

My position is not that I am highly resolved to remove one of those
two functions, or that we should treat it as effectively dead already
and cut our losses by not maintaining it.

It is rather that someday we'll likely want to remove one, if we become
sure that's the right thing to do; and meanwhile, we should maintain
them both. (I haven't even read either function yet; I'm trying to
do my review from the bottom up, and I haven't advanced past the
html class.)

I especially wouldn't want the idea that get_extra_pages_needed()
has received the kiss of death to get in the way of adding a unit
test that could be integrated with it. The need for unit testing
is more important than an eventual desire to reduce duplication.

> GC> [...two pagination implementations...]
> GC> 
> GC> >  The problem is embarrassingly simple, but there is just enough
> GC> > comparisons/divisions in it to make it easy to make a mistake in one of
> GC> > them... It doesn't make it hard, but it does make it error-prone, and 
> even
> GC> > if I'm reasonably certain that I did do it correctly, finally, after
> GC> > writing my test and verifying that it passes for all values, it might 
> still
> GC> > be worth to keep both functions to ensure it doesn't happen again.
> GC> > 
> GC> >  The main negative of doing it I see is that it will make modifying this
> GC> > code more difficult in the future, as it will need to be done in 2 
> (albeit
> GC> > neighboring) places. However I don't believe this code would need to be
> GC> > modified often, so it's probably not a big deal.
> GC> 
> GC> More difficult, for a limited future time only, as long as we decide
> GC> to keep only one after the conclusion of acceptance testing.
> 
>  So should I take this as the expression of the final decision to get rid
> of a separate get_extra_pages_needed()?

No, I can't pronounce a death sentence without even a cursory review.
Even приказ № 227 didn't do that. It's entitled to a tribunal, which
probably won't take place for several months.

> Or should we still keep it for now,
> as that proverbial second chronometer (the difference being that we're not
> out in the sea and that if two implementations disagree again, we can
> always debug them and see which one of them is right).

Yes, absolutely. And if it's easy to add a unit test for even one
of the chronometers, then I think we should do that now: that
might even be almost as good as having three chronometers.

> P.S. Please let me know when you'd like me to stop cc'ing you, AFAICS the
>      mailing list works fine now and I reliably receive all your messages
>      twice.

Please continue for now.

It seemed to me that over the last several days the mailing list
had been sending me only your messages and not my own, so I had
begun to suspect that it was filtering senders, allowing you but
forbidding me. But now that you mention it, I see that I've been
getting nothing from the list--only CCs from you and BCCs from
myself. I'll need to do something about this soon.