Re: [lmi] Unit test for PDF get_extra_pages_needed() function

[Vadim Zeitlin]

On Mon, 12 Feb 2018 10:59:18 +0000 Greg Chicares <address@hidden> wrote:

GC> The number of data rows cannot less than zero, but I think we need to allow
GC> it to equal zero.

 I'm not so sure about this, but we definitely do need to either do this or
add an assertion checking this precondition in the function. This is the
simple part though, and more changes are needed because I hadn't realized
that it were possible to have tables with 0 rows in other places too.

 Just for the record, this trivial change does allow the total number of
rows to be 0:

---------------------------------- >8 --------------------------------------
diff --git a/miscellany.cpp b/miscellany.cpp
index 2ea4baf31..712cedbaa 100644
--- a/miscellany.cpp
+++ b/miscellany.cpp
@@ -196,6 +196,13 @@ std::string iso_8601_datestamp_terse()
     ,int rows_per_page
     )
 {
+    // This special case needs to be handled separately to avoid returning 0
+    // pages from the code below.
+    if (total_rows == 0)
+        {
+        return 1;
+        }
+
     // The caller must check for this precondition because this function is too
     // low-level to be able to handle it correctly, e.g. it can't even use the
     // appropriate error message.
---------------------------------- >8 --------------------------------------

But it's not enough on its own.

GC> To see why...
GC> 
GC> File | New | Census
GC> Census | Edit cell
GC>   on "Payments" tab:
GC>     change "Individual payment" from default 20000 to 1
GC>     OK
GC> Census | Add cell
GC> Census | Print case to PDF
GC> 
GC> The first cell doesn't survive even one month. In the problem domain, we'd
GC> say it lapses immediately; in the solution domain, it has zero data rows.
GC> 
GC> Expected outcome: a PDF is created for each cell, and another one for the
GC> composite. The first cell's PDF reflects its immediate lapse, somehow
GC> (there may be some freedom in how we do that), but that doesn't prevent
GC> the other cells from running or the composite from being generated.
GC> 
GC> Running with '--pyx=only_old_pdf' produces the desired outcome. The first
GC> cell's PDF has zero data rows: it's like a PDF with one data row, except
GC> that it has no data rows.

 I.e. we still want to produce a table, with row headings, even if it
doesn't contain any rows? This seems a bit strange to me, even if
definitely still better than the current behaviour.

GC> Running with '--pyx=only_new_pdf' produces only one PDF, for the first
GC> cell only, and that PDF consists of a single blank page; because this
GC> assertion in pre_render()
GC>           LMI_ASSERT(extra_pages_ >= 0);
GC> fails, processing stops, and no PDF is created for the second cell,
GC> even though that second cell is valid.

 This is one problem: error reporting is very poor and I need to improve it
by giving an actual, user-readable error, saying that the PDF generation
failed and, probably, also removing the incomplete output PDF (I'm not sure
about the latter part, what do you think?).

GC> I'm not sure exactly what we should ideally do for the first cell.
GC> The '--pyx=only_old_pdf' behavior is acceptable because it has been in
GC> production for decades and nobody has complained about it. Probably we
GC> should copy that behavior. As an enhancement, maybe there should be some
GC> indication that it lapsed (we'll have to see what end users think).

 Please let me know if you'd prefer to wait until we know what this
indication should be or whether I should modify the code to output empty
tables.

 In the former case, we still shouldn't allow the number of total rows to
be 0 in page_count(), which is why I started this email by saying that I
wasn't sure I agreed with it.

GC> However, I don't think we can throw an exception and stop processing
GC> the census. If the 900th of 1000 cells lapses before the end of the
GC> first year, end users will have to alter the input to prevent that,
GC> in order to get PDFs for the last 100 cells; but the input file might
GC> be exactly what they want, such that they won't want to alter it.

 This would seem to imply that write_ledger_as_pdf() shouldn't throw any
exceptions at all, but how should it report errors then? Alternatively,
either ledger_emitter::emit_cell() or emit_ledger() could catch any
exceptions and show them, but it's still not clear how should error
reporting work.

 Do you have any idea about what should be the desirable behaviour here?

 Thanks,
VZ