Add uniqueness validation for TableView #5170
Conversation
WalkthroughThe changes involve significant updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@minhtuev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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
|
minhtuev
force-pushed
the
feat/unique-row-action-table
branch
from
1c868d5 to
3012671
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: 4
🧹 Outside diff range and nitpick comments (4)
tests/unittests/operators/view_tests.py (3)
7-15: Consider adding more edge cases and descriptive assertions.The basic column uniqueness test is good, but could be enhanced with:
- Tests for edge cases (empty strings, None values)
- More descriptive assertion messages
def test_table_view_basic(self): table = TableView() table.add_column("column1", label="Column 1") table.add_column("column2", label="Column 2") - assert table.keys() == ["column1", "column2"] + self.assertEqual( + table.keys(), + ["column1", "column2"], + "Table should contain exactly the two added columns" + ) with self.assertRaises(ValueError): table.add_column("column1", label="Column 3") + # Test edge cases + with self.assertRaises(ValueError): + table.add_column("", label="Empty Column") + + with self.assertRaises(ValueError): + table.add_column(None, label="None Column")
18-41: Enhance row action tests with property validation.While the uniqueness check is good, consider validating that the added actions retain their properties correctly.
table.add_row_action( "action1", on_click=mock_on_click, icon="icon1", color="primary", tooltip="Action 1", ) +# Validate action properties +action = table.row_actions[0] # Assuming this accessor exists +self.assertEqual(action.icon, "icon1", "Action icon should match") +self.assertEqual(action.color, "primary", "Action color should match") +self.assertEqual(action.tooltip, "Action 1", "Action tooltip should match")
42-46: Enhance tooltip tests with coordinate validation.Consider adding tests for:
- Invalid coordinates (negative values)
- Tooltip content verification
- Out-of-bounds coordinates
table.add_tooltip(1, 1, "Tooltip 1") table.add_tooltip(1, 2, "Tooltip 2") with self.assertRaises(ValueError): table.add_tooltip(1, 1, "Tooltip 3") + +# Test invalid coordinates +with self.assertRaises(ValueError): + table.add_tooltip(-1, 1, "Invalid Row") + +with self.assertRaises(ValueError): + table.add_tooltip(1, -1, "Invalid Column") + +# Verify tooltip content +tooltip = table.get_tooltip(1, 1) # Assuming this accessor exists +self.assertEqual(tooltip, "Tooltip 1", "Tooltip content should match")fiftyone/operators/types.py (1)
1883-1886: Optimize duplicate checks inTableViewmethodsThe added duplicate checks in
add_column,add_row_action, andadd_tooltipmethods iterate over lists, which could affect performance with large datasets. Consider using sets or dictionaries for faster lookups.Apply this diff to improve performance:
class TableView(View): def __init__(self, **kwargs): super().__init__(**kwargs) self.columns = kwargs.get("columns", []) + self.column_keys = set() self.row_actions = kwargs.get("row_actions", []) + self.row_action_names = set() self.tooltips = kwargs.get("tooltips", []) + self.tooltip_keys = set() def add_column(self, key, **kwargs): if key in self.column_keys: raise ValueError(f"Column with key '{key}' already exists") column = Column(key, **kwargs) self.columns.append(column) + self.column_keys.add(key) return column def add_row_action(self, name, on_click, label=None, icon=None, tooltip=None, **kwargs): if name in self.row_action_names: raise ValueError(f"Action with name '{name}' already exists") row_action = Action( name=name, on_click=on_click, label=label, icon=icon, tooltip=tooltip, **kwargs, ) self.row_actions.append(row_action) + self.row_action_names.add(name) return row_action def add_tooltip(self, row, column, value, **kwargs): + key = (row, column) + if key in self.tooltip_keys: raise ValueError(f"Tooltip for row '{row}' and column '{column}' already exists") tooltip = Tooltip(row=row, column=column, value=value, **kwargs) self.tooltips.append(tooltip) + self.tooltip_keys.add(key) return tooltipAlso applies to: 1894-1897, 1908-1915
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/operators/types.py(5 hunks)tests/unittests/operators/view_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/unittests/operators/view_tests.py
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
🔇 Additional comments (1)
tests/unittests/operators/view_tests.py (1)
1-6: LGTM! Clean test class setup with appropriate imports.
| with self.assertRaises(ValueError): | ||
| table.add_column("column1", label="Column 3") | ||
|
|
||
| mock_on_click = lambda: None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace lambda with a proper function definition.
Per Python style guidelines, avoid assigning lambdas to variables.
-mock_on_click = lambda: None
+def mock_on_click():
+ pass📝 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.
| mock_on_click = lambda: None | |
| def mock_on_click(): | |
| pass |
🧰 Tools
🪛 Ruff
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
| def __init__(self, row, column, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.row = row | ||
| self.column = column | ||
|
|
||
| def clone(self): | ||
| clone = Tooltip(**self._kwargs) | ||
| clone = Tooltip(self.row, self.column, **self._kwargs) | ||
| return clone | ||
|
|
||
| def to_json(self): | ||
| return {**super().to_json()} | ||
| return {**super().to_json(), "row": self.row, "column": self.column} |
There was a problem hiding this comment.
Mandatory row and column parameters in Tooltip constructor may break existing code
The Tooltip class constructor now requires row and column parameters, which could break existing code where Tooltip is instantiated without these parameters. To maintain backward compatibility, consider making row and column optional parameters with default values.
Apply this diff to make row and column optional:
-def __init__(self, row, column, **kwargs):
+def __init__(self, row=None, column=None, **kwargs):
super().__init__(**kwargs)
self.row = row
self.column = column📝 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 __init__(self, row, column, **kwargs): | |
| super().__init__(**kwargs) | |
| self.row = row | |
| self.column = column | |
| def clone(self): | |
| clone = Tooltip(**self._kwargs) | |
| clone = Tooltip(self.row, self.column, **self._kwargs) | |
| return clone | |
| def to_json(self): | |
| return {**super().to_json()} | |
| return {**super().to_json(), "row": self.row, "column": self.column} | |
| def __init__(self, row=None, column=None, **kwargs): | |
| super().__init__(**kwargs) | |
| self.row = row | |
| self.column = column | |
| def clone(self): | |
| clone = Tooltip(self.row, self.column, **self._kwargs) | |
| return clone | |
| def to_json(self): | |
| return {**super().to_json(), "row": self.row, "column": self.column} |
| def __init__(self, name, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.name = name | ||
|
|
||
| def clone(self): | ||
| clone = Action(**self._kwargs) | ||
| clone = Action(self.name, **self._kwargs) | ||
| return clone | ||
|
|
||
| def to_json(self): | ||
| return {**super().to_json()} | ||
|
|
||
| return {**super().to_json(), "name": self.name} | ||
|
|
There was a problem hiding this comment.
Mandatory name parameter in Action constructor may break existing code
The Action class constructor now requires a name parameter, which could potentially break existing code where Action is instantiated without this parameter. To maintain backward compatibility, consider making name an optional parameter with a default value.
Apply this diff to make name optional:
-def __init__(self, name, **kwargs):
+def __init__(self, name=None, **kwargs):
super().__init__(**kwargs)
self.name = name📝 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 __init__(self, name, **kwargs): | |
| super().__init__(**kwargs) | |
| self.name = name | |
| def clone(self): | |
| clone = Action(**self._kwargs) | |
| clone = Action(self.name, **self._kwargs) | |
| return clone | |
| def to_json(self): | |
| return {**super().to_json()} | |
| return {**super().to_json(), "name": self.name} | |
| def __init__(self, name=None, **kwargs): | |
| super().__init__(**kwargs) | |
| self.name = name | |
| def clone(self): | |
| clone = Action(self.name, **self._kwargs) | |
| return clone | |
| def to_json(self): | |
| return {**super().to_json(), "name": self.name} |
minhtuev
force-pushed
the
feat/unique-row-action-table
branch
from
3012671 to
4c430c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/unittests/operators/view_tests.py (3)
7-11: Consider adding assertions for column labelsThe test verifies column keys but doesn't validate that the column labels were set correctly.
def test_table_view_basic(self): table = TableView() table.add_column("column1", label="Column 1") table.add_column("column2", label="Column 2") assert table.keys() == ["column1", "column2"] + assert table.get_column("column1").label == "Column 1" + assert table.get_column("column2").label == "Column 2"
13-14: Enhance error validationConsider validating the error message to ensure it provides clear feedback about the duplicate column.
with self.assertRaises(ValueError): table.add_column("column1", label="Column 3") +with self.assertRaisesRegex(ValueError, "Column 'column1' already exists"): + table.add_column("column1", label="Column 3")
18-40: Consider splitting row action testsThe row action tests could be organized into separate test methods for better clarity and maintainability:
- Test successful addition of row actions
- Test duplicate prevention
- Test action properties validation
Example refactor:
def test_add_row_action_success(self): table = TableView() def mock_on_click(): pass table.add_row_action( "action1", on_click=mock_on_click, icon="icon1", color="primary", tooltip="Action 1", ) action = table.get_row_action("action1") assert action.icon == "icon1" assert action.color == "primary" assert action.tooltip == "Action 1" def test_add_row_action_duplicate(self): table = TableView() def mock_on_click(): pass table.add_row_action("action1", on_click=mock_on_click) with self.assertRaisesRegex(ValueError, "Row action 'action1' already exists"): table.add_row_action("action1", on_click=mock_on_click)fiftyone/operators/types.py (1)
Line range hint
1883-1915: LGTM! The TableView uniqueness validation looks good.The implementation correctly validates uniqueness for columns, row actions, and tooltips. The validation logic is consistent across all three methods and raises appropriate errors when duplicates are found.
Consider enhancing the error messages to provide more context about the duplicate items:
- raise ValueError(f"Column with key '{key}' already exists") + raise ValueError(f"Cannot add column: A column with key '{key}' already exists") - raise ValueError(f"Action with name '{name}' already exists") + raise ValueError(f"Cannot add row action: An action with name '{name}' already exists") - f"Tooltip for row '{row}' and column '{column}' already exists" + f"Cannot add tooltip: A tooltip for row '{row}' and column '{column}' already exists"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/operators/types.py(4 hunks)tests/unittests/operators/view_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/unittests/operators/view_tests.py
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
🔇 Additional comments (3)
tests/unittests/operators/view_tests.py (1)
16-16: Lambda usage was previously flagged
🧰 Tools
🪛 Ruff
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
fiftyone/operators/types.py (2)
1831-1841: LGTM! The Action class changes look good.
The implementation correctly stores and handles the name parameter, which is essential for uniqueness validation in TableView.
1852-1862: LGTM! The Tooltip class changes look good.
The implementation correctly stores and handles the row and column parameters, which are essential for uniqueness validation in TableView.
| table.add_tooltip(1, 1, "Tooltip 1") | ||
| table.add_tooltip(1, 2, "Tooltip 2") | ||
|
|
||
| with self.assertRaises(ValueError): | ||
| table.add_tooltip(1, 1, "Tooltip 3") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance tooltip test coverage
Consider adding the following test cases:
- Validate tooltip content after addition
- Test invalid row/column indices
- Test edge cases (e.g., zero or negative indices)
def test_tooltip_validation(self):
table = TableView()
table.add_column("column1")
# Test successful addition
table.add_tooltip(1, 1, "Tooltip 1")
assert table.get_tooltip(1, 1) == "Tooltip 1"
# Test invalid indices
with self.assertRaises(ValueError):
table.add_tooltip(-1, 1, "Invalid")
with self.assertRaises(ValueError):
table.add_tooltip(1, 0, "Invalid")
minhtuev
force-pushed
the
feat/unique-row-action-table
branch
from
a6fbc02 to
22eb1c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/unittests/operators/types_tests.py (3)
7-15: Consider adding more edge cases to column validation tests.While the basic column validation is well tested, consider adding test cases for:
- Empty column names
- None values
- Special characters in column names
def test_table_view_column_edge_cases(self): table = TableView() # Test empty column name with self.assertRaises(ValueError): table.add_column("", label="Empty") # Test None column name with self.assertRaises(ValueError): table.add_column(None, label="None") # Test special characters table.add_column("column@#$%", label="Special") assert "column@#$%" in table.keys()
18-41: Consider extracting row action creation into a helper method.The row action creation code is repeated. Consider extracting it into a helper method to improve test readability and maintainability.
def _create_row_action(self, action_id, icon, color, tooltip): def mock_on_click(): pass return { "id": action_id, "on_click": mock_on_click, "icon": icon, "color": color, "tooltip": tooltip }
42-46: Add boundary tests for tooltip coordinates.Consider adding test cases for:
- Invalid row/column indices (negative or out of bounds)
- Empty tooltip text
def test_table_view_tooltip_edge_cases(self): table = TableView() table.add_column("column1", label="Column 1") # Test negative indices with self.assertRaises(ValueError): table.add_tooltip(-1, 1, "Invalid") # Test out of bounds with self.assertRaises(ValueError): table.add_tooltip(1, 999, "Invalid") # Test empty tooltip with self.assertRaises(ValueError): table.add_tooltip(1, 1, "")fiftyone/operators/types.py (1)
Line range hint
1878-1929: LGTM! TableView changes prevent duplicates effectivelyThe implementation successfully prevents duplicate entries for columns, row actions, and tooltips. The
_tooltip_mapprovides efficient O(1) lookup for tooltip existence checks.Consider standardizing the error message format across all three validation checks. Currently, they slightly differ:
- raise ValueError(f"Column with key '{key}' already exists") - raise ValueError(f"Action with name '{name}' already exists") - raise ValueError( - f"Tooltip for row '{row}' and column '{column}' already exists" - ) + raise ValueError(f"Duplicate column: key '{key}' already exists") + raise ValueError(f"Duplicate action: name '{name}' already exists") + raise ValueError(f"Duplicate tooltip: position (row '{row}', column '{column}') already exists")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/operators/types.py(4 hunks)tests/unittests/operators/types_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/unittests/operators/types_tests.py
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
🔇 Additional comments (4)
tests/unittests/operators/types_tests.py (2)
1-6: LGTM! Clean test structure and imports.
The test file follows Python testing best practices with clear imports and class structure.
7-46: Verify test coverage aligns with PR objectives.
The test suite effectively covers the uniqueness validation requirements for TableView:
- Column uniqueness ✓
- Row action uniqueness ✓
- Tooltip uniqueness ✓
However, let's verify there are no missing test cases from the implementation.
✅ Verification successful
Test coverage is complete and aligns with implementation
The test suite comprehensively covers all uniqueness validations present in the TableView implementation:
- Column uniqueness: Tests
add_column()duplicate key rejection ✓ - Row action uniqueness: Tests
add_row_action()duplicate name rejection ✓ - Tooltip uniqueness: Tests
add_tooltip()duplicate position rejection ✓
The implementation shows exactly these three validation points raising ValueError, and the test suite has explicit assertions for each case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation-related code in TableView implementation
ast-grep --pattern 'def add_$_(self, $$$):
$$$
raise ValueError($$$)
$$$' fiftyone/operators/types.py
Length of output: 110
Script:
#!/bin/bash
# Let's try a different approach to find the TableView implementation and its validation logic
rg -A 5 "class TableView" fiftyone/operators/types.py
Length of output: 256
Script:
#!/bin/bash
# Search for add_* methods in TableView
rg "def add_" fiftyone/operators/types.py
Length of output: 252
Script:
#!/bin/bash
# Search for ValueError raises in TableView methods
rg "raise ValueError" fiftyone/operators/types.py
Length of output: 419
🧰 Tools
🪛 Ruff
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
fiftyone/operators/types.py (2)
1831-1841: LGTM! Action class changes enforce uniqueness
The mandatory name parameter in the constructor ensures each action has a unique identifier, which aligns with the PR's objective of preventing duplicate row actions. The clone and to_json methods correctly handle the new name field.
1852-1862: LGTM! Tooltip class changes support position-based uniqueness
The mandatory row and column parameters in the constructor ensure each tooltip is uniquely identified by its position. The clone and to_json methods correctly handle these new fields, supporting the TableView's uniqueness validation.
| with self.assertRaises(ValueError): | ||
| table.add_column("column1", label="Column 3") | ||
|
|
||
| mock_on_click = lambda: None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace lambda with a proper function definition.
Following Python best practices and addressing the static analysis warning (E731), replace the lambda with a proper function definition.
- mock_on_click = lambda: None
+ def mock_on_click():
+ pass📝 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.
| mock_on_click = lambda: None | |
| def mock_on_click(): | |
| pass |
🧰 Tools
🪛 Ruff
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/unittests/operators/tableview_tests.py (3)
7-11: Consider splitting the test method into smaller, focused test methods.The current test method covers multiple behaviors (columns, actions, tooltips). Following testing best practices, consider splitting it into separate methods:
test_column_uniquenesstest_row_action_uniquenesstest_tooltip_uniquenessThis improves test maintainability and makes failures more specific.
13-14: Consider adding more column uniqueness test cases.While the basic duplicate column test is good, consider adding tests for:
- Case sensitivity (e.g., "Column1" vs "column1")
- Whitespace handling (e.g., "column1" vs " column1 ")
- Empty or None column names
42-46: Consider adding boundary tests for tooltip coordinates.While the basic tooltip uniqueness test is good, consider adding tests for:
- Invalid coordinates (negative or out of bounds)
- Empty or None tooltip text
- Unicode characters in tooltips
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/unittests/operators/tableview_tests.py(1 hunks)
🧰 Additional context used
🪛 Ruff
tests/unittests/operators/tableview_tests.py
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
🔇 Additional comments (2)
tests/unittests/operators/tableview_tests.py (2)
1-6: LGTM! Clean and focused test class setup.
The imports are minimal and the test class follows good testing practices.
18-40: LGTM! Comprehensive row action testing.
The test thoroughly validates row action uniqueness and includes all relevant properties (icon, color, tooltip).
| with self.assertRaises(ValueError): | ||
| table.add_column("column1", label="Column 3") | ||
|
|
||
| mock_on_click = lambda: None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace lambda with a proper function definition.
Instead of using a lambda expression, define a proper function for better readability and maintainability.
- mock_on_click = lambda: None
+ def mock_on_click():
+ pass📝 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.
| mock_on_click = lambda: None | |
| def mock_on_click(): | |
| pass |
🧰 Tools
🪛 Ruff
16-16: Do not assign a lambda expression, use a def
Rewrite mock_on_click as a def
(E731)
|
Do you know how multiple row actions with the same name are getting added? Would adding this constraints just cause things not to load at all? |
|
@kaixi-wang : we had a PR last week where in QP panel we added multiple actions with the same name and caused a bug |
What changes are proposed in this pull request?
Recently we discovered a rendering bug because two row actions have the same name, so I added uniqueness validation to TableView's properties
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Action,Tooltip, andTableViewclasses for improved functionality and data integrity.TableView.Bug Fixes
TableView.Tests
TableViewclass, covering addition and duplicate handling of columns, row actions, and tooltips.