Utility for adding a path to a sidebar group #5251
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
brimoor
force-pushed
the
add-path-to-sidebar-group
branch
from
26665b9 to
fe56fb3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
fiftyone/core/odm/dataset.py (1)
640-661: Simplify group management logicThe group insertion logic can be simplified and made more maintainable.
- index_group = None - for group in self.sidebar_groups: - if group.name == sidebar_group: - index_group = group - else: - if path in group.paths: - group.paths.remove(path) + # Remove path from all other groups + for group in self.sidebar_groups: + if path in group.paths: + group.paths.remove(path) + + # Find or create target group + index_group = next( + (group for group in self.sidebar_groups if group.name == sidebar_group), + None + ) if index_group is None: index_group = SidebarGroupDocument(name=sidebar_group) - - insert_after = None - if after_group is not None: - for i, group in enumerate(self.sidebar_groups): - if group.name == after_group: - insert_after = i - - if insert_after is None: - self.sidebar_groups.append(index_group) - else: - self.sidebar_groups.insert(insert_after + 1, index_group) + insert_index = ( + next( + (i for i, group in enumerate(self.sidebar_groups) + if group.name == after_group), + None + ) + ) + insert_index = (insert_index + 1) if insert_index is not None else len(self.sidebar_groups) + self.sidebar_groups.insert(insert_index, index_group)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/core/dataset.py(2 hunks)fiftyone/core/odm/dataset.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fiftyone/core/dataset.py
| def _add_path_to_sidebar_group( | ||
| self, | ||
| path, | ||
| sidebar_group, | ||
| after_group=None, | ||
| dataset=None, | ||
| ): | ||
| if self.sidebar_groups is None: | ||
| if dataset is None: | ||
| return | ||
|
|
||
| self.sidebar_groups = self.default_sidebar_groups(dataset) | ||
|
|
||
| index_group = None | ||
| for group in self.sidebar_groups: | ||
| if group.name == sidebar_group: | ||
| index_group = group | ||
| else: | ||
| if path in group.paths: | ||
| group.paths.remove(path) | ||
|
|
||
| if index_group is None: | ||
| index_group = SidebarGroupDocument(name=sidebar_group) | ||
|
|
||
| insert_after = None | ||
| if after_group is not None: | ||
| for i, group in enumerate(self.sidebar_groups): | ||
| if group.name == after_group: | ||
| insert_after = i | ||
|
|
||
| if insert_after is None: | ||
| self.sidebar_groups.append(index_group) | ||
| else: | ||
| self.sidebar_groups.insert(insert_after + 1, index_group) | ||
|
|
||
| if path not in index_group.paths: | ||
| index_group.paths.append(path) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Direct manipulation of sidebar groups found in mutation.py
The new utility method _add_path_to_sidebar_group should be used instead of direct manipulation of sidebar_groups found in fiftyone/server/mutation.py. This ensures consistent handling of sidebar group management across the codebase.
fiftyone/server/mutation.py: Replace direct assignment ofsidebar_groupswith calls to_add_path_to_sidebar_group
🔗 Analysis chain
Verify usage pattern in the codebase
Let's ensure this new utility method is being used consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for direct manipulation of sidebar_groups that should use this new utility.
# Test: Search for direct manipulation of sidebar_groups. Expect: No direct manipulation.
# Look for direct manipulation of sidebar_groups
rg -A 5 'sidebar_groups\s*=' --type py
# Look for path manipulation in sidebar groups
rg -A 5 'paths\.append|paths\.remove' --type py
Length of output: 18187
| def _add_path_to_sidebar_group( | ||
| self, | ||
| path, | ||
| sidebar_group, | ||
| after_group=None, | ||
| dataset=None, | ||
| ): | ||
| if self.sidebar_groups is None: | ||
| if dataset is None: | ||
| return | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and improve error handling
The method should validate the path parameter and provide a more informative error message when dataset is required but not provided.
def _add_path_to_sidebar_group(
self,
path,
sidebar_group,
after_group=None,
dataset=None,
):
+ if not path or not isinstance(path, str):
+ raise ValueError("path must be a non-empty string")
+ if not sidebar_group or not isinstance(sidebar_group, str):
+ raise ValueError("sidebar_group must be a non-empty string")
if self.sidebar_groups is None:
if dataset is None:
- return
+ raise ValueError("dataset parameter is required when sidebar_groups is not initialized")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _add_path_to_sidebar_group( | |
| self, | |
| path, | |
| sidebar_group, | |
| after_group=None, | |
| dataset=None, | |
| ): | |
| if self.sidebar_groups is None: | |
| if dataset is None: | |
| return | |
| def _add_path_to_sidebar_group( | |
| self, | |
| path, | |
| sidebar_group, | |
| after_group=None, | |
| dataset=None, | |
| ): | |
| if not path or not isinstance(path, str): | |
| raise ValueError("path must be a non-empty string") | |
| if not sidebar_group or not isinstance(sidebar_group, str): | |
| raise ValueError("sidebar_group must be a non-empty string") | |
| if self.sidebar_groups is None: | |
| if dataset is None: | |
| raise ValueError("dataset parameter is required when sidebar_groups is not initialized") |
brimoor
requested review from
Br2850,
manivoxel51 and
lanzhenw
and removed request for
a team
@manivoxel51 yeah I noticed that too: it's a UI issue that exists before and after this PR. If you refresh your browser, |
Refactored the sidebar group-related code in
Dataset.create_summary_field()into aDatasetAppConfig._add_path_to_sidebar_group()utility so it can be reused by other methods in the future.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
create_summary_fieldmethod for better clarity and organization.