split relation utils#1088
Conversation
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that splits the large FlexibleDataModelRelationUtils object into smaller, more focused utility objects: Convertors, RowDataExtractors, Validators, and a new orchestrating FlexibleDataModelRelationUtils. This is an excellent change that greatly improves the code's structure, maintainability, and readability. The new organization is logical and makes the purpose of each component much clearer.
I've found one critical issue regarding a type mismatch during the construction of instance data, which would likely cause a compilation failure. I have provided a detailed comment with a suggested fix. Once that is addressed, this refactoring will be in great shape.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1088 +/- ##
=======================================
Coverage 85.17% 85.17%
=======================================
Files 47 50 +3
Lines 3102 3102
Branches 507 498 -9
=======================================
Hits 2642 2642
Misses 460 460
🚀 New features to boost your workflow:
|
|
This pull request seems a bit stale.. Shall we invite more to the party? |
Wondering if something like this (minimum changes here) is viable to help clean things up a bit. It's a bit difficult to understand what exactly is included in "FlexibleDataModelsRelationUtils" and this split along Convertors, RowDataExtractors and Validators should help a bit... But it might be an absolute pain to review so not sure if worth it.
This is not a major improvement, just a (hopefully) relatively logical split of a large class into 4 more targeted ones, should be no functional changes. My biggest concern is how annoying it's going to be to review for a reviewer :/