search

europeana / metis-schema

3 stars 0 forks source link

MET-6139 Move rdf convertion classes from metis-framework. #14

Closed stzanakis closed 1 month ago

stzanakis commented 1 month ago

@jeortizquan This is a first step of the code migration. In this first iteration Tests(if any) are not migrated, packaging organization not optimized, reported issues not resolved. With the first review, we just agree on the direction and then we continue the implementation on this PR.

@jochen-vermeulen In the end the todo I mentioned was actually from you in the end. So perhaps you have a say in this one as well?

jochen-vermeulen commented 1 month ago

@stzanakis Just wondering about separation of concerns and all that. I think when it comes to the metis-schema module, we should try to keep it focused only on functionality that is specific to the RDF schema and converting to/from it.

It seems that you're proposing to move media-processing specific code to the metis schema (e.g. extract the hasView, isShownAt, etc. from records). Reading back my TODO I believe my thoughts at the time was to transfer just the code that deserializes to a Document. The code specific to media processing could stay.

Specifically, in RdfDeserializer I marked the code that I think should stay in the media processing module. I think that RdfNamespaceContext, ResourceEntry, RdfResourceEntry, RdfXpathContstants, UrlType and perhaps other bits of code then also should stay in media processing?

Bits and pieces like XPathExpressionWrapper may perhaps stay and could then be used throughout the codebase? Or perhaps it should move to metis common instead?

What do you think?

jochen-vermeulen commented 1 month ago

And I think my TODO suggested something else. Namely, removing duplicate code between RdfConversionUtils already existing in Metis Schema on the one hand, and RdfDeserializer and RdfSerializer on the other. I marked two such cases, but I think there are many more opportunities for merging these three classes.

stzanakis commented 1 month ago

@stzanakis Just wondering about separation of concerns and all that. I think when it comes to the metis-schema module, we should try to keep it focused only on functionality that is specific to the RDF schema and converting to/from it.

It seems that you're proposing to move media-processing specific code to the metis schema (e.g. extract the hasView, isShownAt, etc. from records). Reading back my TODO I believe my thoughts at the time was to transfer just the code that deserializes to a Document. The code specific to media processing could stay.

Specifically, in RdfDeserializer I marked the code that I think should stay in the media processing module. I think that RdfNamespaceContext, ResourceEntry, RdfResourceEntry, RdfXpathContstants, UrlType and perhaps other bits of code then also should stay in media processing?

Bits and pieces like XPathExpressionWrapper may perhaps stay and could then be used throughout the codebase? Or perhaps it should move to metis common instead?

What do you think?

Okay. As I mentioned earlier, the TODO comment was clear I just thought taking it a step further and move more code that could be potentially used by other projects. That being said I was a bit hesitant of this impactful change. So that clears it I guess. Thanks for having a look. Closing this to rework on a smaller update and perhaps some renaming on the media library.