-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new Springshare sources Libguides and Research Databases #99
Conversation
Why these changes are being introduced: Adding two new TIMDEX sources, Libguies and Research Databases, which need to be transformed into TIMDEX records. How this addresses that need: Libguides and Research Databases (called AZ List) are products of Springshare, both come from the same OAI-PMH feed, and therefore share a very similar DC schema. For this reason, a parent "SpringshareOaiDc" transformer was created that extracts most, if not all, of the optional TIMDEX fields from the source XML. Then, two transformers -- Libguides and ResearchDatabases -- extend this class. At this time, they do not modify the parent class's behavior at all, but were kept distinct in the event they may have slight modifications over time. The following source names were decided on: - libguides: Libguides - researchdatabases: Research Databases, aka the AZ List An ADR has been included that captures the discussions for naming these sources. Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-227 Additional maintenance: * Updated dependencies
Why these changes are being introduced: After some discussion, opted to follow similar patterns with sources like zenodo for extending transformer classes. How this addresses that need: Instead of a dedicated method for extending the optional fields, this follows an established pattern of adding methods within the get_optional_fields method that can be overridden (e.g. get_dates()) Side effects of this change: * None Relevant ticket(s): * None
Why these changes are being introduced: * Adding these new data sources prompted some discussion and decision about source names that would benefit from being recorded How this addresses that need: * Establishes an ADR structure in this project and records the source name discussions as the first added Side effects of this change: * None Relevant ticket(s): * None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, happy to chat about any of these suggestions!
Why these changes are being introduced: * Updates stemming from code review for PR #99 How this addresses that need: * updated test naming * additional tests for OaiDc get_dates() and get_links() hooks * fallback on default citation generator * ensure usage of str(<BS4_element.string) for memory concerns Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-227
Why these changes are being introduced: * Updates stemming from code review for PR #99 How this addresses that need: * updated test naming * additional tests for OaiDc get_dates() and get_links() hooks * fallback on default citation generator * ensure usage of str(<BS4_element.string) for memory concerns Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-227
a87c413
to
982f569
Compare
@ehanson8 - whenever you have time, for your review, changes have been pushed to address the comments above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of comments but this is minor stuff
* removing docstrings in unit tests in favor of more meaningful test names * refactor libguides and researchdatabases tests into springshare * make error and warning logging message patterns from transform consistent with other transforms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, 3 comments!
- DRYed up fixture prefixes - normalized variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go, great work!
What does this PR do?
This PR adds two new sources that
transmogrifier
can handle:libguides
andresearchdatabases
.The overall approach is this:
OaiDc
SpringshareOaiDc
that extendsOaiDc
SpringshareOaiDc
transformer as defined in the source configurationsThis PR also updates dependencies.
How can a reviewer manually see the effects of these changes?
Example of
libguides
transformation:Example of
researchdatabases
transformation:Delete this section if it isn't applicable to the PR.
What are the relevant tickets?
Developer
Code Reviewer
(not just this pull request message)
Includes new or updated dependencies?
YES