Add 1st set of DspaceDim field methods #194

ehanson8 · 2024-06-17T19:58:57Z

Purpose and background context

Refactors the first set of DspaceDim fields as separate field methods.

How can a reviewer manually see the effects of these changes?

Run the following command to see that the DspaceDim transform still transforms a source file and skips an invalid content type according to the Whoas transform:

pipenv run transform -i tests/fixtures/dspace/dspace_dim_records.xml -o output/dspace-transformed-records.json -s whoas

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/TIMX-282

Developer

All new ENV is documented in README
All new ENV has been added to staging and production environments
All related Jira tickets are linked in commit message(s)
Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

The commit message is clear and follows our guidelines (not just this PR message)
There are appropriate tests covering any new functionality
The provided documentation is sufficient for understanding any new functionality introduced
Any manual tests have been performed and verified
New dependencies are appropriate or there were no changes

Why these changes are being introduced: * Refactor DspaceDim to use field methods How this addresses that need: * Add create_dspace_dim_source_record_stub function to DspaceDim test module * Rename param xml > source_record * Add field methods and associated private methods for alternate_titles, citation, and content_type * Refactor Whoas.get_content_type method to raise exception for more explicit flow control * Add unit tests for new field methods Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-282

ghukill

All in all, I think it looks great! Left a couple comments about yielding + list extending, but I'm finding these really easy to parse and reason about. Nice work on the changes, and the PR chunking.

ghukill · 2024-06-17T20:49:09Z

transmogrifier/sources/xml/dspace_dim.py

+            yield timdex.Contributor(
+                value=str(creator.string),
+                kind="Creator",
+            )


Just curious (and same for _get_contributors...() below), why the yield?

On one hand, I kind of like it: no need for list comprehensions, just yield Contributor objects as they are found. On the other hand: feels a bit different than other places?

FWIW, I do kind of like the yield? Wonder -- not pressing by any means -- if opportunity to move in this direction for other methods as we encounter them?

I was actually copying the pattern from Datacite, I believe we've used it in a few places but we could check for consistency when this refactor is done

Yeah, those might make for a couple of nice PRs when the first pass is over: "Normalize yield..", "Normalize BS4 string casting...", etc. Though I think by and large things are pretty good.

transmogrifier/sources/xml/dspace_dim.py

ghukill · 2024-06-17T20:52:10Z

transmogrifier/sources/xml/dspace_dim.py

+        alternate_titles = [
+            timdex.AlternateTitle(
+                value=str(alternate_title.string),
+                kind=alternate_title["qualifier"],
+            )
+            for alternate_title in source_record.find_all(
+                "dim:field", element="title", string=True
+            )
+            if alternate_title.get("qualifier")
+        ]


Whether we ultimately keep BS4, or move into Xpath/lxml, I'm finding this pattern increasingly easy to read.

ghukill · 2024-06-17T20:53:05Z

transmogrifier/sources/xml/dspace_dim.py

+                        kind="coverage",
+                    )
+            else:
+                yield timdex.Date(note=coverage_value, kind="coverage")


Ah, more yields! Maybe I just missed them in other transforms. Digging this pattern.

transmogrifier/sources/xml/dspace_dim.py

ghukill · 2024-06-17T20:55:55Z

tests/sources/xml/test_dspace_dim.py

+        timdex.Contributor(value="Jamerson, James", kind="Creator"),
+        timdex.Contributor(
+            value="LaFountain, James R.",
+            kind="author",
+        ),
+        timdex.Contributor(
+            value="Oldenbourg, Rudolf",
+            kind="author",
+        ),
+    ]


Noting now, but don't think we should make these changes in this refactor, but this feels like a good opportunity at some point to normalized kind values. Seeing Creator (title cased) and author (lowercase). I know author comes from the XML itself, and it's always risky to programatically convert case.

Maybe normalization is some kind of post-processing / post record transform? Again, not needed now, just noting for the future.

I was almost certain we had a Jira ticket for this but we don't, I'll create an issue

Thanks for creating the ticket. Maybe when this field method refactor is done, before we launch into orchestration, we could meet and survey the tickets created? Get a feel for anything we may want to also tackle during the orchestration? or after?

Before sounds like a good idea!

ghukill · 2024-06-17T20:56:32Z

transmogrifier/sources/xml/whoas.py

+        if cls.valid_content_types(content_types):
+            return content_types
+        message = f'Record skipped based on content type: "{content_types}"'
+        raise SkippedRecordEvent(message, cls.get_source_record_id(source_record))


* Remove list() from .extend calls

ehanson8 · 2024-06-18T13:41:14Z

@ghukill Had to add some type hinting in get_contributors after removing `list(...)

ghukill · 2024-06-18T13:44:18Z

@ghukill Had to add some type hinting in get_contributors after removing `list(...)

Ahhh, didn't think of that. Please feel free to disregard the removal of list(...). As mentioned, really not super consequential, just feeling like a time to comment on patterns given the refactoring work. But both make sense, and maybe the linting / typing advantages outweigh anything else!

ehanson8 · 2024-06-18T13:50:34Z

I prefer it without list(...) so I'm fine with a occasional inter-method type hinting as a trade-off. I'll clean up the Datacite class in a separate commit in my next PR

ghukill

LGTM 👍

ghukill · 2024-06-18T13:50:53Z

transmogrifier/sources/xml/dspace_dim.py

+
+    @classmethod
+    def get_contributors(cls, source_record: Tag) -> list[timdex.Contributor] | None:
+        contributors: list[timdex.Contributor] = []


I like it. I'm also a little surprised that mypy needed this to understand what what was coming from those methods which are both clearly typed.

ghukill · 2024-06-18T13:51:57Z

transmogrifier/sources/xml/dspace_dim.py

+            yield timdex.Contributor(
+                value=str(creator.string),
+                kind="Creator",
+            )


Yeah, those might make for a couple of nice PRs when the first pass is over: "Normalize yield..", "Normalize BS4 string casting...", etc. Though I think by and large things are pretty good.

ehanson8 requested review from ghukill and jonavellecuerdo June 17, 2024 19:58

ghukill approved these changes Jun 17, 2024

View reviewed changes

Updates based on discussion in PR #194

cc10b97

* Remove list() from .extend calls

ehanson8 requested a review from ghukill June 18, 2024 13:41

ghukill approved these changes Jun 18, 2024

View reviewed changes

ehanson8 merged commit 00d2024 into main Jun 18, 2024
3 of 5 checks passed

ehanson8 deleted the TIMX-282-dspacedim-fmr-origins branch June 18, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 1st set of DspaceDim field methods #194

Add 1st set of DspaceDim field methods #194

ehanson8 commented Jun 17, 2024 •

edited

Loading

ghukill left a comment

ghukill Jun 17, 2024

ehanson8 Jun 18, 2024

ghukill Jun 18, 2024

ghukill Jun 17, 2024

ghukill Jun 17, 2024

ghukill Jun 17, 2024

ehanson8 Jun 18, 2024

ghukill Jun 18, 2024

ehanson8 Jun 18, 2024

ghukill Jun 17, 2024

ehanson8 commented Jun 18, 2024

ghukill commented Jun 18, 2024

ehanson8 commented Jun 18, 2024 •

edited

Loading

ghukill left a comment

ghukill Jun 18, 2024

ghukill Jun 18, 2024

Add 1st set of DspaceDim field methods #194

Add 1st set of DspaceDim field methods #194

Conversation

ehanson8 commented Jun 17, 2024 • edited Loading

Purpose and background context

How can a reviewer manually see the effects of these changes?

Includes new or updated dependencies?

Changes expectations for external applications?

What are the relevant tickets?

Developer

Code Reviewer(s)

ghukill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehanson8 commented Jun 18, 2024

ghukill commented Jun 18, 2024

ehanson8 commented Jun 18, 2024 • edited Loading

ghukill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehanson8 commented Jun 17, 2024 •

edited

Loading

ehanson8 commented Jun 18, 2024 •

edited

Loading