Skip to content
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

Timx 288 marc field method refactor 3 #202

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

jonavellecuerdo
Copy link
Contributor

@jonavellecuerdo jonavellecuerdo commented Jul 25, 2024

Purpose and background context

Field method refactor for transform class Marc (Part 3).

  • Added field methods and corresponding unit tests for the following fields: [identifiers, languages, literary_form, locations, notes]

Note(s)

  1. The new get_locations field method correctly retrieves the characters 008/15-17; the old code previously only retrieved characters 008/15-16, effectively ignoring timdex.Location(kind="Place of Publication") for records with 3-letter codes.

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

  1. Run make test and verify all unit tests are passing.
  2. Run CLI command
    pipenv run transform -i tests/fixtures/marc/marc_record_all_fields.xml -o output/marc-transformed-records.json -s alma
    
    Output:
    2024-07-25 17:51:03,644 INFO transmogrifier.cli.main(): Logger 'root' configured with level=INFO
    2024-07-25 17:51:03,644 INFO transmogrifier.cli.main(): No Sentry DSN found, exceptions will not be sent to Sentry
    2024-07-25 17:51:03,644 INFO transmogrifier.cli.main(): Running transform for source alma
    2024-07-25 17:51:04,176 INFO transmogrifier.cli.main(): Completed transform, total records processed: 1, transformed records: 1, skipped records: 0, deleted records: 0
    2024-07-25 17:51:04,176 INFO transmogrifier.cli.main(): Total time to complete transform: 0:00:00.531964
    

Includes new or updated dependencies?

NO

Changes expectations for external applications?

NO

What are the relevant tickets?

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

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

@jonavellecuerdo jonavellecuerdo self-assigned this Jul 25, 2024
@jonavellecuerdo jonavellecuerdo marked this pull request as ready for review July 26, 2024 11:48
Copy link
Contributor

@ghukill ghukill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me for the most part. Only thing shy of immediate approval is standardizing Marc.<method> or cls.<method>, where I'd lean towards the latter.

transmogrifier/sources/xml/marc.py Show resolved Hide resolved
transmogrifier/sources/xml/marc.py Outdated Show resolved Hide resolved
transmogrifier/sources/xml/marc.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions but looks great!

transmogrifier/sources/xml/marc.py Show resolved Hide resolved
transmogrifier/sources/xml/marc.py Show resolved Hide resolved
transmogrifier/sources/xml/marc.py Show resolved Hide resolved
transmogrifier/sources/xml/marc.py Outdated Show resolved Hide resolved
transmogrifier/sources/xml/marc.py Show resolved Hide resolved
Why these changes are being introduced:
* These updates are required to implement the architecture described
in the following ADR: https://github.com/MITLibraries/transmogrifier/blob/main/docs/adrs/0005-field-methods.md

How this addresses that need:
* Added field methods and corresponding unit tests:
  identifiers, languages, literary_form, locations,
  notes

Side effects of this change:
* None

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/TIMX-288
@jonavellecuerdo jonavellecuerdo force-pushed the TIMX-288-marc-field-method-refactor-3 branch from 891cbce to 9b42ff3 Compare July 30, 2024 14:16
@jonavellecuerdo jonavellecuerdo merged commit a25fd0a into main Jul 30, 2024
5 checks passed
@jonavellecuerdo jonavellecuerdo deleted the TIMX-288-marc-field-method-refactor-3 branch July 30, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants