-
Notifications
You must be signed in to change notification settings - Fork 596
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
add support for setting view by name and slug #4178
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v0.23.7 #4178 +/- ##
===================================================
- Coverage 28.85% 28.84% -0.01%
===================================================
Files 766 768 +2
Lines 97004 97032 +28
Branches 1118 1120 +2
===================================================
Hits 27988 27988
- Misses 69016 69044 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Array.isArray(savedViews) && | ||
savedViews.find((view) => name === view.name); | ||
if (!savedView) { | ||
throw new Error(`Saved view with name "${name}" does not exist.`); |
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.
@imanjra do we have a JS implementation of the to_slug() utility?
Asking because, if so, then you could just do name = to_slug(name)
here and then name
could effectively be either a name or slug. The public interface validates that any new saved view name must be unique both as a name
and a slug
, so it wouldn't cause any views to be "unreachable".
One consequence of such an approach is that the error message could only read:
`Saved view with name or slug "${name}" does not exist`
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.
Good idea. Yeah, we do have it. I'll use that util here
1c5a479
to
deb35e0
Compare
953cd5f
to
e72d574
Compare
e72d574
to
19f3974
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.
LGTM, tested by running voxel51/fiftyone-plugins#115 and also tested a case where a saved view didn't exist and I saw the error notification as expected. Very nice!
What changes are proposed in this pull request?
add support for setting view by name and slug
Usage: setting a view using the slug of a saved view
Usage: setting a view using the name of a saved view
How is this patch tested? If it is not, please explain why.
Using a test operator
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
add support for setting view by name and slug
What areas of FiftyOne does this PR affect?
fiftyone
Python library changes