-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Active presets init #806
Conversation
Unit test report (Pydantic 1.x)189 tests 189 ✅ 5s ⏱️ Results for commit 4e85b64. ♻️ This comment has been updated with latest results. |
Unit test report ((Pydantic 2.x)189 tests 189 ✅ 6s ⏱️ Results for commit 4e85b64. ♻️ This comment has been updated with latest results. |
1d709f0
to
7caa1b8
Compare
@@ -51,11 +55,9 @@ class CreatePresetParams(BaseDTO): | |||
|
|||
class CreatePresetPayload(BaseDTO): | |||
name: str | |||
description: Optional[str] = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a real parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is emblematic of Active/ Index being a bad overload of two very similiar functionalitys branching at random points. For some reason, we do have a description that we don't render
encord/orm/filter_preset.py
Outdated
class FilterDefinition(BaseDTO): | ||
filters: List[Dict] = Field(default_factory=list) | ||
|
||
|
||
class FilterPresetDefinition(BaseDTO): | ||
local_filters: Dict[str, FilterDefinition] = Field( | ||
default_factory=lambda: {str(uuid.UUID(int=0)): FilterDefinition()}, alias="local_filters" | ||
default_factory=lambda: {str(uuid.UUID(int=0)): FilterDefinition()}, alias="localFilters" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somewhat concerning that this changed. Same is shared between Active and Index and Index already used so this can't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked that that change is compatible with master so we're chilling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we are camel-to-str on DTO anyway so this is un-necessary
) | ||
global_filters: FilterDefinition = Field(default_factory=FilterDefinition, alias="global_filters") | ||
|
||
@dto_validator(mode="after") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lengthy commit message describing why this had to go
c5fe4db
to
e1db9d3
Compare
Reason: When recieving presets from Active, we will filter out presets to be scoped on a project basis. This means that we can have empty presets when having local filters that only come from a different project
…nd we should follow convention where possible
e1db9d3
to
4e85b64
Compare
Introduction and Explanation
Add active preset functions to the sdk
JIRA
Fixes https://linear.app/encord/issue/EC-5247/sdk-changes-to-support-active-presets
Documentation
Docs are being added as well