Move metrics config to base extractor config#484
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #484 +/- ##
==========================================
+ Coverage 80.59% 80.63% +0.04%
==========================================
Files 43 43
Lines 4194 4194
==========================================
+ Hits 3380 3382 +2
+ Misses 814 812 -2
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the configuration by moving MetricsConfig into the base ExtractorConfig. The changes in the core logic and model definitions are clear and well-implemented. However, I have some concerns about the modifications made to the test files. In several places, assertions checking the exact number of errors have been removed. While this might be intended to reduce test flakiness, it makes the tests less robust by potentially hiding other issues and can lead to uninformative IndexError failures if no errors are returned. I've added specific comments with suggestions for more resilient test assertions.
|
given the gemini comments, I tend to aggree, you should at least check that the |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration structure by moving MetricsConfig into the base ExtractorConfig. This is a sound architectural improvement that makes the configuration more logical and cohesive. The changes are applied consistently throughout the codebase, including necessary updates to the Extractor class and its corresponding tests. The implementation is correct and I did not find any issues.
|
🦄 |
|
@Hmnt39 remember about the PR labels |
This PR moves the
MetricsConfigto baseExtractorConfiginstead ofFullConfig(generated by Runtime).