Use 572 digital collections class - #275
Conversation
Why these changes are being introduced: * A transform class is needed for adding Digital Collections to TIMDEX How this addresses that need: * Add DigitalCollections class derived from DspaceDim with methods for source-specific fields and corresponding unit tests * Add digitalcollections to Config.SOURCES * Fix linting issue in Transformer class * Update dependencies Side effects of this change: * NA Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-572
There was a problem hiding this comment.
Pull request overview
Adds a dedicated DigitalCollections XML transformer to support the Digital Collections source (USE-572), wiring it into the source configuration and validating expected field mappings with new unit tests.
Changes:
- Introduces
DigitalCollectionstransformer subclassingDspaceDimwith Digital Collections-specific field overrides. - Registers the new source in
SOURCESconfig and adds a comprehensive test suite for the transformer. - Minor refactor in exclusion-list loading and updates Ruff lint ignore list.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| transmogrifier/sources/xml/digital_collections.py | New DigitalCollections transformer extending DspaceDim with source-specific mappings. |
| transmogrifier/sources/transformer.py | Small refactor to cast exclusion_list_path to str for smart_open. |
| transmogrifier/config.py | Adds new digitalcollections source entry pointing to the new transformer and base URL. |
| tests/sources/xml/test_digital_collections.py | Adds unit tests covering the new transformer’s field extraction behavior. |
| pyproject.toml | Expands Ruff ignore list for additional lint rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ghukill
left a comment
ghukill
left a comment
There was a problem hiding this comment.
Left a couple of comments, but overall looks good to me!
I'd be remiss if I didn't ask: we're pretty sure we want to go with DIM? I can appreciate the metadata side of things, but I'm a little bummed that we'll have the more minimal DIM files vs METS which do seem to carry more information about the object (surprisingly). Either way, that's likely a question/discussion out of scope for this PR. I can live with DIM for sure.
While I did leave some comments, I'd like to go ahead with an approval review seeing as I'm out Thursday/Friday of this week and don't want to hold things up. Can always take more passes or revisit things, but everything here looks good in situ.
|
@ghukill Valid question about |
* Update get_funding_information method type hinting * Use variables in get_notes method to improve readability * Add whitespace and comments to get_subjects method to improve readability * Add docstring to get_summary method to clarify what is mapped to that field
jonavellecuerdo
left a comment
jonavellecuerdo
left a comment
There was a problem hiding this comment.
Looks good to me! :)
Purpose and background context
Adding
DigitalCollectionstransformer class to extendDspaceDimclass based on this metadata mapping. Feedback from stakeholders has been requested so I may delay merging until they have time to weigh-in.How can a reviewer manually see the effects of these changes?
Review Digital Collections records in our
Dev1UI. Note: Reload the page if you get a 500 error since theDev1goes to sleep if unused.Sample search for "medicine" in the Digital Collections tab
Includes new or updated dependencies?
YES
Changes expectations for external applications?
YES -
timdex-pipeline-lambdasandtimdex-index-managerwill require small updates, PRs coming after these changes are mergedWhat are the relevant tickets?
Code review