Add validator to handle extractable features types #107
Conversation
This file contains hidden or 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
cmttt
requested review from
a team,
BinaryFiddler,
EXBreder,
ayubun,
haileyok and
jaredmiller13
as code owners
cmttt
force-pushed
the
ls/extracted-feature-validator
branch
from
ccc5489 to
e236c8a
Compare
BinaryFiddler
approved these changes
Jan 8, 2026
EXBreder
approved these changes
Jan 8, 2026
| from osprey.engine.language_types.entities import EntityT | ||
| from osprey.engine.udf.type_helpers import to_display_str | ||
|
|
||
| from ..base_validator import SourceValidator |
Collaborator
There was a problem hiding this comment.
use the full osprey.engine.* path for consistency.
| To use a custom type without extraction, prefix the variable name with `_` to make | ||
| it a local variable: | ||
| _VC1 = CustomUdf(entity=TestUser) # OK - not extracted | ||
| VC1 = CustomUdf(entity=TestUser) # ERROR - would be extracted |
Collaborator
There was a problem hiding this comment.
coming in without too much context, and I do remember this is a thing we do, but its kinda a hack in the first place, assigning some semantic meaning to the name of a variable is pretty unexpected.
Definitely think this validator is a positive, but we should think about what we want a better API to look like in the future (in_the_backlog.gif)
| non_none_args = [a for a in args if a is not type(None)] # noqa: E721 | ||
| # Optional is a Union with exactly one non-None type | ||
| if len(non_none_args) == 1: | ||
| return _is_extractable_type(non_none_args[0]) |
| # Check Optional[T] (which is Union[T, None] at runtime) | ||
| if typing_inspect.is_union_type(t): | ||
| args = typing_inspect.get_args(t) | ||
| non_none_args = [a for a in args if a is not type(None)] # noqa: E721 |
Collaborator
There was a problem hiding this comment.
Technically this also allows Union[T], but I guess we dont really care
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.
UDFs that return a custom type will break the UI's extracted features -> values mapping, so we need a validator that checks that we aren't implementing any rules that return a custom type.