Refactor _AnnotationMapper
#9020
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SpecLad
force-pushed
the
refactor-annotation-mapper
branch
from
67e00a2 to
23e406e
Compare
This has two main goals: 1. Move all validation that does not depend on the task we're annotating from `_AnnotationMapper` and to `DetectorFunctionSpec`. This a) splits the code into more manageable chunks, and b) works better with the agent workflow, because the CLI `function` commands can reject an invalid function spec immediately (rather than waiting until the first request). (Frankly, I don't know why I didn't put this code in `DetectorFunctionSpec` in the first place, given that it implements the restrictions described in the docstring...) Validating the spec upon construction requires that the spec is not modified afterwards. This has always been my intention, but to make it clearer, state it explicitly in the docstring. 2. Restructure the rest of the code into smaller functions. This should not introduce any differences in behavior.
SpecLad
force-pushed
the
refactor-annotation-mapper
branch
from
23e406e to
602ee28
Compare
SpecLad
requested review from
zhiltsov-max,
bsekachev and
nmanovic
as code owners
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9020 +/- ##
===========================================
- Coverage 73.39% 73.39% -0.01%
===========================================
Files 416 417 +1
Lines 44198 44226 +28
Branches 3860 3860
===========================================
+ Hits 32440 32458 +18
- Misses 11758 11768 +10
|
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and context
This has two main goals:
Move all validation that does not depend on the task we're annotating from
_AnnotationMapperand toDetectorFunctionSpec. This a) splits the code into more manageable chunks, and b) works better with the agent workflow, because the CLIfunctioncommands can reject an invalid function spec immediately (rather than waiting until the first request).(Frankly, I don't know why I didn't put this code in
DetectorFunctionSpecin the first place, given that it implements the restrictions described in the docstring...)Validating the spec upon construction requires that the spec is not modified afterwards. This has always been my intention, but to make it clearer, state it explicitly in the docstring.
Restructure the rest of the code into smaller functions. This should not introduce any differences in behavior.
How has this been tested?
Checklist
developbranch[ ] I have linked related issues (see GitHub docs)License
Feel free to contact the maintainers if that's a concern.