Timx 291 orchestration #205
Conversation
* Update get_valid_title, get_source_link, and get_timdex_record_id to instance methods in Transformer, JSONTransformer, and XMLTransformer classes
* Update get_source_link and get_timdex_record_id to instance methods in Aardvark and SpringshareOaiDC classes * Update corresponding unit tests
Why these changes are being introduced: * Refactoring this method fundamentally simplifies the application and allows for the removal of several methods How this addresses that need: * Refactor _transform method * Add generate_derived_fields and get_optional_field_methods methods * Add get_required_field_names and get_optional_field_names methods to TimdexRecord class Side effects of this change: * All transform classes with need to be updated in subsequent commits Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-291
* Remove get_required_fields method from Transformer, JSONTransformer, and XMLTransformer classes * Remove unit tests for this method
* Some field methods omitted the source_record param because they delivered static values. This caused issues with the new orchestration so an unused param was added.
* Refactor create_dates_and_locations_from_publishers and create_locations_from_spatial_subjects method to use TimdexRecord instances * Call methods in generate_derived_fields * Update corresponding unit tests
* Refactor generate_citation to use a TimdexRecord instance * Shift call into generate_derived_fields method * Update corresponding unit tests
* Refactor MITAardvark.get_rights to instance method in order to access source attribute * Update corresponding unit tests
* Remove get_optional_fields method from all classes * Remove unit tests for this method
* Refactor derived field methods to return iterators * Split create_dates_and_locations_from_publishers method based on object type * Refactor generate_derived_fields method to accommodate whether attributes exist or not
* Remove if not record block from __next__ method as it relates to an outdated method of flow control *Rename _transform to transform and remove transform methods from JSONTransformer and XMLTransformer * Refactor generate_derived_fields and get_optional_field_methods * Remove try/except block from transform method
|
@ghukill @jonavellecuerdo I'm making this PR official, thank you both so very much for all of the discussion! |
There was a problem hiding this comment.
Overall, I think this looks superb! Nice work @ehanson8. Thanks for taking a complex topic, faciliating discussions around changes, and materializing all that in this PR. While large, it's easy to follow, and the changes feel in-step with things we've been discussing.
As promised, saved some granular and nit picky questions / comments for this "real" PR. Functionality-wise, really no concerns: I see this operating as intended, and tests and linting is looking good. Comments are mostly stylistic, method organization, and some questions that may be better spun off as separate issues.
Curious thoughts on a couple of the questions, but close to approval with or without any changes.
|
@ehanson8 The additional updates are looking good to me! Waiting for response re: ordering (had a follow-up question for @ghukill ) and think one change request still needs to be implemented. |
* Remove obsolete lxml comments and linting overrides * Update Transformer.transform method docstring * Refactor Transformer.get_optional_field_methods
|
@ghukill @jonavellecuerdo Changes pushed! |
Purpose and background context
Update the application's orchestration to streamline the code and improve maintainability
How can a reviewer manually see the effects of these changes?
Run a JSON and an XML transform to ensure that records are still transformed successfully after the orchestration refactor:
Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)