[Koha-bugs] [Bug 11368] Add script to import Lexile scores

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11368

--- Comment #13 from Kyle M Hall <kyle.m.hall at gmail.com> ---
(In reply to Jonathan Druart from comment #11)
> Comment on attachment 39182 [details] [review]
> Bug 11368 - Add script to import Lexile scores
> 
> Review of attachment 39182 [details] [review]:
> -----------------------------------------------------------------
> 
> I get this warning:
> DBIx::Class::ResultSet::search_rs(): search( %condition ) is deprecated, use
> search( \%condition ) instead at misc/migration_tools/import_lexile.pl line
> 147

Fixed!

> 
> ::: misc/migration_tools/import_lexile.pl
> @@ +65,5 @@
> > +    'source=s'               => $subfield_source,
> > +    'source-value=s'         => $subfield_source_value,
> > +);
> > +
> > +my $usage = << 'ENDUSAGE';
> 
> Should not we use Pod::Usage?
> 
> @@ +82,5 @@
> > +
> > +ENDUSAGE
> > +
> > +unless ($file) {
> > +    say $usage;
> 
> Prefer to call pod2usage.

I don't think the developer guidelines require pod2usage. That being said, I'd
welcome a followup!

> @@ +88,5 @@
> > +}
> > +
> > +my $schema = Koha::Database->new()->schema();
> > +
> > +my $csv = Text::CSV->new( { binary => 1, sep_char => "\t" } )
> 
> Shouldn't we use the pref "separator" and add an option to specify another
> one?

The file format is fixed with tabs as the separator, so we should definitely
not use the syspref here.

> @@ +121,5 @@
> > +            push( @isbns, $row->{$_} );
> > +            eval { push( @isbns, GetVariationsOfISBN( $row->{$_} ) ) };
> > +        }
> > +    }
> > +    @isbns = grep( $_, @isbns );
> 
> Not sure to undestand what this does :)

This line filters out any 'empty' isbns.

> @@ +142,5 @@
> > +
> > +        if ($verbose) {
> > +            say "Found matching record! Biblionumber: $biblionumber";
> > +
> > +            if ( $verbose > 2 ) {
> 
> You should mention this in the POD.

Done!

-- 
You are receiving this mail because:
You are watching all bug changes.