[OPEN-ILS-DEV] MFHD Patch - Third Try

[Dan Scott]

On Fri, 2009-11-20 at 17:25 -0500, Dan Wells wrote:
> Hello all,
> 
> Here is yet another version of my MFHD patch.  There are no actual code changes from what has already been reviewed by Dan and David.  The new patch:
>   - restores deleted debug comments
>   - follows more carefully the whitespace patches as recently applied in trunk
>   - uses svn diff as run from my svn root (the trunk directory)

Thanks Dan. On the whole, I'm pretty happy with the direction you're
taking things, based on my understanding of MFHD (I'm certainly no
expert!). I do have a few questions / concerns / comments, though, that
should be easy for you to respond to.

In general:

  * It would be awesome (but, with the exception of David's MFHD code,
largely unprecedented, so don't feel bad!) to have tests that go
hand-in-hand with the code that you're introducing, so we can see the
expected input & behaviour. But I live in a dream world...

MFHD.pm:
  * active_captions() subroutine
    * In your comments, you mention that the standard is hazy about how
multiple active patterns should be handled. I've looked in
http://www.loc.gov/marc/holdings/hd853855.html and
http://www.loc.gov/marc/holdings/echdcntf.html and don't see the term
"active" used in either of those places, so perhaps there's a
terminology mismatch, or I'm looking in the wrong place.

  * generate_predictions() subroutine
    * It would be useful to have more comments describing the approach
you're taking to generate the predictions
    * I don't understand why perl2JSON is being invoked on one element
of the returned array. OpenSRF normally handles the conversion to/from
JSON for us, so I don't think you added it for OpenSRF service support. 
    * The new dependency on OpenSRF::Utils::JSON also complicates reuse
of the the MFHD modules. Until now, they could be picked up and reused
by / collaborated on with another project (hello Koha!) because they
haven't had any dependencies on the rest of the OpenSRF or Evergreen
libraries.

MFHDParser.pm:
  * No concerns

MFHD/Caption.pm:
  * next() subroutine:
    * We enter into a for() loop over a..m, and then, if the holding
that was passed in is compressed and open-ended, we return from the
subroutine. It looks like we could test for compression + open-endedness
and return immediately before we even enter the for() loop, no?

MFHD/Holdings.pm:

  * notes() subroutine:
    * There's a lot going on in here that doesn't appear to be in use
yet but will be built on subsequently. I'm not sure how you're planning
on calling it, but perhaps passing the notes list by reference, and
making it the first argument, rather than passing the list by value as
the second argument, would avoid some of the shift / test / unshift
gyrations? Hmm, well, it sort of looks like it tries to handle being
called with the notes passed as a reference, too, as there are also
tests for whether the first element of @notes is itself a reference. Ah,
it also acts as a getter, I see. Would it be cleaner as separate
get_notes() / set_notes() subroutines? In any case, some comments would
be helpful here.
    * Also, a typo: 'specifiying'

  * format() subroutine:
    * I'm not too happy having the English "Note: " hardcoded here. In
format_textual_holdings in MFHDParser.pm, I delimited the holdings and
the note via a single dash ('$holdings - $note)', but I suppose there is
a possibility of the single dash introducing some confusion with
compressed holdings. Maybe a double-dash as a delimiter?

  * renaming match() to matches() subroutine
    * In my opinion, this wasn't really a necessary change.

  * chron_to_date() subroutine:
    * Would be nice to have a comment describing what the 3 elements of
the @chron_start and @chron_end lists represent.

  * _uncombine() subroutine:
    * Typo of the sub name in carp(): "uncombine"

Well, I've kind of run out of steam at this point. I did want to send
some feedback your way, based on reading the patches (I haven't had time
yet to actually run the code, although I would like to compare the
formatting of the holdings for our serials to check for significant
differences). I haven't seen any showstoppers, and I think the more
concerns that I've expressed could be addressed in subsequent patches if
this patch were committed to trunk as-is.

I'm not sure what the requirements are for DCOs when a patch has been
resubmitted. I'll ask for one anyway, just in case.

Thanks very much for this, Dan - both for the code and the patience that
you've demonstrated waiting for a review of your patches. And for
helping spur a better perltidy format. And for writing up your serials
thoughts at
http://evergreen-ils.org/dokuwiki/doku.php?id=acq:serials:basic_predicting_and_receiving (I'll probably have some comments about that next week, if you're interested).