-
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
misc: Revert removal of description used in Index #827
misc: Revert removal of description used in Index #827
Conversation
Unit test report ((Pydantic 2.x)193 tests 193 ✅ 6s ⏱️ Results for commit 79e8285. ♻️ This comment has been updated with latest results. |
Unit test report (Pydantic 1.x)193 tests 193 ✅ 5s ⏱️ Results for commit 79e8285. ♻️ This comment has been updated with latest results. |
@@ -56,6 +56,7 @@ class CreatePresetParams(BaseDTO): | |||
class CreatePresetPayload(BaseDTO): | |||
name: str | |||
filter_preset_json: Dict | |||
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.
It may be worth refactoring this to have two separate Preset Types dependant on whether we are Active or Index. But that would be more code. Including a description erroneously in the payload for Active is kinda sad
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.
probably better to have separate types
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.
Done
00d4e49
to
79e8285
Compare
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.
Stale comments
self._client.patch( | ||
f"index/presets/{self.uuid}", | ||
params=None, | ||
payload=payload, | ||
result_type=None, | ||
) | ||
self._preset_instance.name = name or self.name |
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.
Make it so we update local cache on successful update. Note that it falls after the request which will throw if unsuccessful.
if not filter_preset.local_filters and not filter_preset.global_filters: | ||
raise EncordException("We require there to be a non-zero number of filters in a preset") | ||
payload = CreatePresetPayload( | ||
payload = IndexCreatePresetPayload( | ||
name=name, | ||
filter_preset_json=filter_preset.to_dict(), |
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.
Note: Above we parse the input, just to immediately de-serialise it. Would it not be better to have it take a more structured object in the signature. Cc @Encord-davids. Theoretically breaks the interface but it's all somewhat fubar anyway
Introduction and Explanation
Index does apparently have a description. We should not have such bad duplicated functionality without tests