Field method ADR #174
Field method ADR #174
Conversation
Why these changes are being introduced: * Discussion is needed on a proposed field method refactor How this addresses that need: * Add ADR for proposed field method refactor Side effects of this change: * None Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-275
There was a problem hiding this comment.
A couple of comments, but overall, I think it's great. Succinct, direct, understandable. I think the "Consequences" section is particularly good, explaining the value add from this work in an easily digestable way.
As commented, I think it might benefit from a brief explanation of a "field method" early on, to solidify that term and understanding throughout.
ADR text, approved. And actual ADR decision, I'm in favor!
docs/adrs/0005-field-methods.md
Outdated
|
|
||
| Currently much of the transformation logic for each source is found in the `get_optional_fields` method which extracts data for most of the fields. This results in very large methods as well as inefficient orchestration and testing. | ||
|
|
||
| A field method approach would address many of these complications. All of the required fields have dedicated field methods as well as some of the complex fields that are called by `get_optional_fields`. This refactor can be seen as an extension of that approach. |
There was a problem hiding this comment.
Though I understand what a "field method" is, I think that term may need to be introduced here if it's going to be used.
That way, it also makes sense exactly what it means here,
All of the required fields have dedicated field methods...
Though I think the Decision and Consequences section do a good job of defining what a "field method" is, I believe it should be up here as well (or at least first), to help with this section.
There was a problem hiding this comment.
Great suggestion, let me know if this is a good definition
|
|
||
|
|
||
| ## Consequences | ||
| Field methods should ultimately simplfy the source transforms, and the overall application, by providing an easily repeatable structure with method docstrings offering additional context for the more complicated logic required for some fields. |
There was a problem hiding this comment.
Nice, concise, accurate, and I agree!
| ## Consequences | ||
| Field methods should ultimately simplfy the source transforms, and the overall application, by providing an easily repeatable structure with method docstrings offering additional context for the more complicated logic required for some fields. | ||
|
|
||
| Testing should also be significantly improved. The current testing suite is built around very large fixtures and unit tests meant to encompass the expected data scenarios for each source's records. They are not easy to parse and can be a barrier to new developers trying to understand the application. Separate tests for each field method should simplfy the testing infrastructure and make it easier to maintain. |
There was a problem hiding this comment.
👍 - again, this is well stated.
docs/adrs/0005-field-methods.md
Outdated
|
|
||
| Testing should also be significantly improved. The current testing suite is built around very large fixtures and unit tests meant to encompass the expected data scenarios for each source's records. They are not easy to parse and can be a barrier to new developers trying to understand the application. Separate tests for each field method should simplfy the testing infrastructure and make it easier to maintain. | ||
|
|
||
| Logging and associated analytics should also be easier to manage and provide a better view of the data transformed by this application. |
There was a problem hiding this comment.
Given the focus of analytics and metrics in current strategic plans, and TIMDEX roadmapping, I think it could be helpful to provide an example here, even if minimal.
One that comes to mind is easily (because it is technically possible before a refactor) surfacing metrics about particular fields. Ignoring for the moment where the metrics are sent, it now seems possible to wrap the "field methods" during orchestration of the transform, thereby allowing for a consistent and shared way of reporting errors or outlier behavior when transforming fields.
There was a problem hiding this comment.
I am stealing some of this language : )
|
It is not clear why a |
This happened to me the other day as well; just intermittent coveralls hiccups it seems. |
|
|
||
| Currently much of the transformation logic for each source is found in the `get_optional_fields` method which extracts data for all optional fields. This results in very large methods as well as inefficient orchestration and testing. | ||
|
|
||
| A field method approach would address many of these complications, where each `TimdexRecord` field has a corresponding method containing all of the logic related to extracting and formatting the values for that field. All of the required fields have dedicated field methods as well as some of the complex fields that are called by `get_optional_fields`. This refactor can be seen as an extension of that approach. |
There was a problem hiding this comment.
Dig it. Thanks for explaining what a "field method" is here.
There was a problem hiding this comment.
I agree! Awesome description.
|
|
||
| Testing should also be significantly improved. The current testing suite is built around very large fixtures and unit tests meant to encompass the expected data scenarios for each source's records. They are not easy to parse and can be a barrier to new developers trying to understand the application. Separate tests for each field method should simplfy the testing infrastructure and make it easier to maintain. | ||
|
|
||
| Logging and associated analytics should also be easier to manage and provide a better view of the data transformed by this application. During orchestration of the transform, the field methods will provide a consistent and shared way of reporting errors or outlier behavior when transforming fields. |
|
|
||
| Currently much of the transformation logic for each source is found in the `get_optional_fields` method which extracts data for all optional fields. This results in very large methods as well as inefficient orchestration and testing. | ||
|
|
||
| A field method approach would address many of these complications, where each `TimdexRecord` field has a corresponding method containing all of the logic related to extracting and formatting the values for that field. All of the required fields have dedicated field methods as well as some of the complex fields that are called by `get_optional_fields`. This refactor can be seen as an extension of that approach. |
There was a problem hiding this comment.
I agree! Awesome description.
Purpose and background context
ADR for discussion of a field method refactor
Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
Developer
Code Reviewer(s)