Description
Tomás Cohen Arazi (tcohen)
2023-09-19 12:35:05 UTC
Created attachment 155907 [details] [review] Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc This patch introduces a new pattern for the different ->get_<thing> methods we've been adding. The aim is that code will look more like: my $metadata_extractor = Koha::MetadataExtractor->new; while ( my $biblio = $biblios->next ) { my $record = $biblio->record; my $schema = $biblio->record_schema; $data->{$biblio->id}->{normalized_upc} = $metadata_extractor->get_normalized_upc( { record => $record, schema => $schema } ); $data->{$biblio->id}->{normalized_ean} = $metadata_extractor->get_normalized_ean( { record => $record, schema => $schema } ); } The key is that we are actually reusing the MARC::Record, and code for each schema is organized cleanly so easier to maintain. For the class names, I chose to add the 'MARC' name in the path, so we don't need to refactor anything if we want to add support for another serialization formats. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ qa -c 1 => SUCCESS: Tests pass! 3. Sign off :-D I'm intrigued by this one. It looks practical and high performance. However, it also feels a bit unnatural. It's more natural to have something like $metadata->get_normalized_upc. -- Looking at your patches, I notice that Koha::MetadataExtractor::MARC::MARC21 doesn't have any internal state, so it doesn't necessarily need to require object instantiation. And the only internal state for Koha::MetadataExtractor is a list of stateless objects. So I have a suggestion for an alternative using static class methods. It would be just as practical and high performance, but it would be more concise. It would look like this: $biblio_metadata->extractor->get_normalized_upc({ record => $record }) I suppose that might look a little confusing, so you could do something like 'my $extractor = $biblio_metadata->get_extractor' and then use '$extractor->get_normalized_upc({ record => $record })'. The Koha::Biblio::Metadata object has the "schema" to-hand, so the "extractor" method would dynamically return the name of the schema-specific extractor class for that metadata record, and then "get_normalized_upc" would be called as a static class method like Koha::MetadataExtractor::MARC::MARC21->get_normalized_upc({ record => $record }). It's very concise and has all the same benefits of your current implementation. I'll include a little proof-of-concept as a comment. #!/usr/bin/perl use strict; use warnings; my $biblio = Local::Biblio->new({type => 'Test'}); warn $biblio; warn $biblio->extractor; warn $biblio->extractor->awesome; package Local::Biblio; sub new { my ($class,$args) = @_; $args = {} unless $args; my $self = bless($args,$class); return $self; } sub extractor { my ($self) = @_; my $type = $self->{type}; return "Local::Extractor::$type"; } package Local::Extractor; sub new { my ($class) = @_; my $self = {}; return bless $self, $class; } package Local::Extractor::Test; sub awesome { my ($class,$args) = @_; return "$class == awesome"; } 1; #!/usr/bin/perl use strict; use warnings; my $biblio = Local::Biblio->new({type => 'Test'}); warn $biblio; warn $biblio->extractor; warn $biblio->extractor->awesome; package Local::Biblio; sub new { my ($class,$args) = @_; $args = {} unless $args; my $self = bless($args,$class); return $self; } sub extractor { my ($self) = @_; my $type = $self->{type}; return "Local::Extractor::$type"; } package Local::Extractor::Test; sub awesome { my ($class,$args) = @_; return "$class == awesome"; } 1; Another option would be to implement get_normalized_upc() in Koha::Biblio::Metadata and have it do an internal call to that "extractor" function. That could be useful for caching and storing internal state in Koha::Biblio::Metadata if we wanted. David and all, I'm happy to have the ball roll and have all your ideas. I'm not attached to this particular code in any way. I'm just generally not sure about the approach we have taken, and this is the best I have come up with. opac-detail.pl has calls to: GetNormalizedEAN GetNormalizedISBN GetNormalizedOCLCNumber GetNormalizedUPC GetMarcControlnumber GetMarcISBN GetMarcISSN GetMarcSeries GetMarcSubjects GetMarcUrls About object orientation: I think I just didn't want to type long names for each call (considering we might have mixes of schemas, even). I would agree my code could just be passed a Koha::Biblio::Metadata object instead, which *should* cache the calculated MARC::Record object in some way. The ambivalence between Koha::Biblio and Koha::Biblio::Metadata has always smelled. It become evident when we were developing the /biblios endpoint. And the reason I filed bug 14645 8 years ago (LOL, just remembered). What do we want to type/read? I personally would like something like: my $extractor = Koha::Biblio::MetadataExtractor->new({biblio => $biblio}); Koha::MetadataExtractor retrieves the record and the schema from the biblio object. then simply retrieve using: my $upc = $extractor->[get_]normalized_upc; And we will see for authorities later, we don't need it for now. (In reply to Jonathan Druart from comment #7) > What do we want to type/read? > > I personally would like something like: > > my $extractor = Koha::Biblio::MetadataExtractor->new({biblio => $biblio}); > > Koha::MetadataExtractor retrieves the record and the schema from the biblio > object. > > then simply retrieve using: > > my $upc = $extractor->[get_]normalized_upc; I like that. (In reply to Jonathan Druart from comment #7) > What do we want to type/read? > > I personally would like something like: > > my $extractor = Koha::Biblio::MetadataExtractor->new({biblio => $biblio}); Thinking about it again today and I suppose I just wonder about all the different objects we'll have to do similar things. Koha::Biblio Koha::Biblio::Metadata Koha::Biblio::MetadataExtractor Koha::MetadataRecord Koha::RecordProcessor I can't keep them all straight. I hadn't even heard of Koha::MetadataRecord until yesterday. Something like "get_normalized_upc" should conceptually be a method of Koha::Biblio or Koha::Biblio::Metadata. But it would be a bit annoying to have a lot of if/elsif/else statements in methods like "get_normalized_upc" based on a schema condition. So I can see the value of Koha::Biblio::MetadataExtractor and having separate subclasses for different schemas, as it is a clean way of providing schema specific method overrides. But surely "hiding" it within Koha::Biblio or Koha::Biblio::Metadata would be better reading/writing for developers. Created attachment 156565 [details] [review] Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc This patch introduces a new pattern for the different ->get_<thing> methods we've been adding. The aim is that code will look more like: my $metadata_extractor = Koha::MetadataExtractor->new; while ( my $biblio = $biblios->next ) { my $record = $biblio->record; my $schema = $biblio->record_schema; $data->{$biblio->id}->{normalized_upc} = $metadata_extractor->get_normalized_upc( { record => $record, schema => $schema } ); $data->{$biblio->id}->{normalized_ean} = $metadata_extractor->get_normalized_ean( { record => $record, schema => $schema } ); } The key is that we are actually reusing the MARC::Record, and code for each schema is organized cleanly so easier to maintain. For the class names, I chose to add the 'MARC' name in the path, so we don't need to refactor anything if we want to add support for another serialization formats. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ qa -c 1 => SUCCESS: Tests pass! 3. Sign off :-D Created attachment 156566 [details] [review] Bug 34828: Move to Koha::Biblio::Metadata::Extractor We are not directly linked with the other Koha::Metadata* packages. Better to isolate this under Koha::Biblio::Metadata (at least for now). Created attachment 156567 [details] [review] Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC and inherit from it The new usage is now: 1. With a Koha::Biblio object my $extractor = Koha::Biblio::Metadata::Extractor->new({biblio => $biblio}); $extractor->get_normalized_upc; or 2. With a MARC::Record my $extractor = Koha::Biblio::Metadata::Extractor->new({metadata=> $biblio->metadata->record}); $extractor->get_normalized_upc; Note that there are "Inconsistent hierarchy during C3 merge of class" warnings raised by the QA script. We could remove them by replacing the 'use' by 'require' in Koha::Biblio::Metadata::Extractor::MARC (in ->new) but that's suboptimal. Created attachment 156568 [details] [review] Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor Hey, what do you think of this change? Note that I don't think we should add this new concept/module/abstraction/complexity if there is no plan to move it further (i.e. a goal/roadmap we share to replace what is C4::Koha and Koha::Biblio that is parsing MARC::Record to get data). Also note that bug 23235 become even more relevant now... (In reply to Jonathan Druart from comment #14) > Hey, what do you think of this change? I like it. That's how I thought we could use it. > Note that I don't think we should add this new > concept/module/abstraction/complexity if there is no plan to move it further > (i.e. a goal/roadmap we share to replace what is C4::Koha and Koha::Biblio > that is parsing MARC::Record to get data). It is done, I don't think having 'MARC' in the package name hurts, as of UNIMARC and MARC21, the different packages make sense, though. > Also note that bug 23235 become even more relevant now... Yeah, probably. Setting to NSO? Created attachment 156575 [details] [review] Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc This patch introduces a new pattern for the different ->get_<thing> methods we've been adding. The aim is that code will look more like: my $metadata_extractor = Koha::MetadataExtractor->new; while ( my $biblio = $biblios->next ) { my $record = $biblio->record; my $schema = $biblio->record_schema; $data->{$biblio->id}->{normalized_upc} = $metadata_extractor->get_normalized_upc( { record => $record, schema => $schema } ); $data->{$biblio->id}->{normalized_ean} = $metadata_extractor->get_normalized_ean( { record => $record, schema => $schema } ); } The key is that we are actually reusing the MARC::Record, and code for each schema is organized cleanly so easier to maintain. For the class names, I chose to add the 'MARC' name in the path, so we don't need to refactor anything if we want to add support for another serialization formats. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ qa -c 1 => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Created attachment 156576 [details] [review] Bug 34828: Move to Koha::Biblio::Metadata::Extractor We are not directly linked with the other Koha::Metadata* packages. Better to isolate this under Koha::Biblio::Metadata (at least for now). Signed-off-by: David Nind <david@davidnind.com> Created attachment 156577 [details] [review] Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC and inherit from it The new usage is now: 1. With a Koha::Biblio object my $extractor = Koha::Biblio::Metadata::Extractor->new({biblio => $biblio}); $extractor->get_normalized_upc; or 2. With a MARC::Record my $extractor = Koha::Biblio::Metadata::Extractor->new({metadata=> $biblio->metadata->record}); $extractor->get_normalized_upc; Note that there are "Inconsistent hierarchy during C3 merge of class" warnings raised by the QA script. We could remove them by replacing the 'use' by 'require' in Koha::Biblio::Metadata::Extractor::MARC (in ->new) but that's suboptimal. Signed-off-by: David Nind <david@davidnind.com> Created attachment 156578 [details] [review] Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor Signed-off-by: David Nind <david@davidnind.com> I've signed off, as the test plan works. However, I don't really understand the changes as I'm not a developer - please feel free to change it back to needs sign off if that is required. Created attachment 156743 [details] [review] Bug 34828: Introduce Koha::MetadataExtractor and ->get_normalized_upc This patch introduces a new pattern for the different ->get_<thing> methods we've been adding. The aim is that code will look more like: my $metadata_extractor = Koha::MetadataExtractor->new; while ( my $biblio = $biblios->next ) { my $record = $biblio->record; my $schema = $biblio->record_schema; $data->{$biblio->id}->{normalized_upc} = $metadata_extractor->get_normalized_upc( { record => $record, schema => $schema } ); $data->{$biblio->id}->{normalized_ean} = $metadata_extractor->get_normalized_ean( { record => $record, schema => $schema } ); } The key is that we are actually reusing the MARC::Record, and code for each schema is organized cleanly so easier to maintain. For the class names, I chose to add the 'MARC' name in the path, so we don't need to refactor anything if we want to add support for another serialization formats. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ qa -c 1 => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 156744 [details] [review] Bug 34828: Move to Koha::Biblio::Metadata::Extractor We are not directly linked with the other Koha::Metadata* packages. Better to isolate this under Koha::Biblio::Metadata (at least for now). Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 156745 [details] [review] Bug 34828: Add Koha::Biblio::Metadata::Extractor::MARC and inherit from it The new usage is now: 1. With a Koha::Biblio object my $extractor = Koha::Biblio::Metadata::Extractor->new({biblio => $biblio}); $extractor->get_normalized_upc; or 2. With a MARC::Record my $extractor = Koha::Biblio::Metadata::Extractor->new({metadata=> $biblio->metadata->record}); $extractor->get_normalized_upc; Note that there are "Inconsistent hierarchy during C3 merge of class" warnings raised by the QA script. We could remove them by replacing the 'use' by 'require' in Koha::Biblio::Metadata::Extractor::MARC (in ->new) but that's suboptimal. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 156746 [details] [review] Bug 34828: Make normalized_oclc uses Koha::Biblio::Metadata::Extractor Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Thanks Jonathan and David^2. I really feel we now have a better path for future refactoring efforts. Pushed to master for 23.11. Nice work everyone, thanks! Looks great. Maybe there will be a plugin hook for this. Enhancement not pushed to 23.05.x Created attachment 156789 [details] [review] Bug 34828: Remove useless variable Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 156791 [details] [review] Bug 34828: Fix test Follow-ups pushed to master. Thanks! |