Timx 282 dspacedim fmr judgment day #196
Conversation
Why these changes are being introduced: * Finish refactoring DspaceDim to use field methods How this addresses that need: * Add field methods and associated private methods for funding_information, identifiers, languages, links, locations, notes, publishers, related_items, rights, subjects, and summary * Add unit tests for new field methods Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-282
| <dim:field mdschema="dc" element="relation" | ||
| >A low resolution version of this movie was published.</dim:field> | ||
| <dim:field mdschema="dc" element="relation" qualifier="ispartofseries" | ||
| >International Association of Aquatic and Marine Science</dim:field> | ||
| <dim:field mdschema="dc" element="relation" qualifier="uri" | ||
| >https://doi.org/10.1002/2016JB013228</dim:field> |
There was a problem hiding this comment.
This is a very minor nit, not a required or even suggested change, but I think personally I would find these kind of stubs easier to read if the elements were one line?
e.g.,
source_record = create_dspace_dim_source_record_stub(
"""
<dim:field mdschema="dc" element="relation">A low resolution version of this movie was published.</dim:field>
<dim:field mdschema="dc" element="relation" qualifier="ispartofseries">International Association of Aquatic and Marine Science</dim:field>
<dim:field mdschema="dc" element="relation" qualifier="uri">https://doi.org/10.1002/2016JB013228</dim:field>
"""
)I understand this breaks line length linting... but the readability might be worth it sometimes.
Again, not requesting a change -- particularly as it's probably fairly common across transforms -- but noting to propose that it's an option when we want.
There was a problem hiding this comment.
We can talk about that, not a big fan of sidescrolling, but not sure if I'm the only one so a later discussion is warranted!
There was a problem hiding this comment.
I think I prefer side-scrolling 🤔. If the XML tags themselves are long, I would also prefer:
<tag>
[some long string]
</tag>
This is a constant struggle in my head while writing fixtures/tests for the field method refactor. 😅
There was a problem hiding this comment.
Got it, yes, let's discuss further!
| return [ | ||
| timdex.Identifier( | ||
| value=str(identifier.string), | ||
| kind=identifier.get("qualifier") or "Not specified", | ||
| ) | ||
| for identifier in source_record.find_all( | ||
| "dim:field", element="identifier", string=True | ||
| ) | ||
| if identifier.get("qualifier") != "citation" | ||
| ] or None |
There was a problem hiding this comment.
Using this as an example, but I'm finding these easy to scan now. I do wonder if it's because I've seen quite a few now: list comprehension + filtering clause + or for some values. It's complex, but I think once you take the time to understand one, you kind of understand the rest in the transform.
When we get into orchestration, and all FMR is done, it might be worth looking at some of these and see if some helper utilities can be constructed.
There was a problem hiding this comment.
Yes! I agree that this pattern is ripe for further abstraction. Created an issue for this!
Purpose and background context
Finish refactoring
DspaceDimclass to use field methods.How can a reviewer manually see the effects of these changes?
Run the following command to see that the
DspaceDimtransform still transforms a source file:Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)