Additional MITAardvark methods #109
Conversation
Why these changes are being introduced: * Additonal methods are needed for the MITAardvark class How this addresses that need: * Update get_optional_fields method to include format and summary values and add corresponding unit test * Add get_alternate_titles, get_contributors, get_notes, get_publication_information, and get_rights field methods along with calls in get_optional_fields and corresponding unit tests * Update aardvark_record_all_fields fixture to include new fields Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-54
There was a problem hiding this comment.
Overall, looking real good.
Left a couple comments about list comprehension patterns, maybe an opportunity to collapse those down a bit. And one comment about Rights.uri that may or may not throw errors (though tests aren't catching it if so...).
But looking good, easy to follow.
* Update dct_license_sm field in aardvark fixture to properly reflect expected value * Refactor get_alternate_titles, get contributors, and get_notes methods for more efficient processing * Update get_rights method to properly process expected value of dct_license_sm
|
Pushed new commit @ghukill @jonavellecuerdo |
| rights.extend( | ||
| [ | ||
| timdex.Rights(uri=rights_uri_value) | ||
| for rights_uri_value in source_record.get("dct_license_sm", []) | ||
| ] | ||
| ) |
tests/conftest.py
Outdated
| JsonTransformer.parse_source_file( | ||
| "tests/fixtures/aardvark/aardvark_record_all_fields.jsonl" | ||
| ) | ||
| return JsonTransformer.parse_source_file( |
There was a problem hiding this comment.
Hmm, how do we feel about renaming:
JsonTransformer -> JSONTransformer--
using common capitalized abbreviations when naming functions?
Asking @ghukill , too! 🤓
There was a problem hiding this comment.
This is following the capitalization pattern of classes like Ead where is was decided at some point in the past that all-caps class names like EAD weren't preferred. Not sure if that is following some linting principle but I don't have strong feelings either way about the capitalization
There was a problem hiding this comment.
Just my two cents: I prefer, and have used, all caps like JSONTransformer or SQSClient as examples. But I noticed Transmog follows a pretty strict CamelCasing for classes. Doesn't feel like a deal breaker either way to me, but maybe leaning towards consistency in the project.
There was a problem hiding this comment.
Hmm, I don't think it's based on a linting rule--since it seemed pass without error in geo-harvester, but I do think CamelCasing is generally preferred. I'm okay with it being consistent just within the repo at the least, so it's fine as is!
There was a problem hiding this comment.
Honestly, I have to stop myself typing JSON rather than Json every time and we aren't consistent about it in other repos so I'll switch it to JSONTransformer and XMLTransformer
tests/conftest.py
Outdated
| JsonTransformer.parse_source_file( | ||
| "tests/fixtures/aardvark/aardvark_record_all_fields.jsonl" | ||
| ) | ||
| return JsonTransformer.parse_source_file( |
There was a problem hiding this comment.
Hmm, I don't think it's based on a linting rule--since it seemed pass without error in geo-harvester, but I do think CamelCasing is generally preferred. I'm okay with it being consistent just within the repo at the least, so it's fine as is!
* Update dct_license_sm field in aardvark fixture to properly reflect expected value * Refactor get_alternate_titles, get contributors, and get_notes methods for more efficient processing * Update get_rights method to properly process expected value of dct_license_sm
Helpful background context
Further updates to expand the
MITAardvarkclass. The remaining fields can likely be covered in the next PRHow can a reviewer manually see the effects of these changes?
Updates to the CLI are needed before this can be tested manually.
What are the relevant tickets?
Developer
Code Reviewer
(not just this pull request message)
Includes new or updated dependencies?
NO