Skip to content
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.

Sign up for GitHub

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 uniqueness validation for TableView #5170

Merged
merged 3 commits into from
Dec 11, 2024
Merged

Add uniqueness validation for TableView #5170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
37315d2
Added uniqueness validation to TableView
Nov 21, 2024
22eb1c3
More efficient check
Nov 21, 2024
2a224ab
Renamed to tableview_tests
Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 31 additions & 8 deletions fiftyone/operators/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1828,16 +1828,18 @@ class Action(View):
on_click: the operator to execute when the action is clicked
"""

def __init__(self, **kwargs):
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}

Comment on lines +1831 to +1841
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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}


class Tooltip(View):
"""A tooltip (currently supported only in a :class:`TableView`).

Expand All @@ -1847,15 +1849,17 @@ class Tooltip(View):
column: the column of the tooltip
"""

def __init__(self, **kwargs):
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}
Comment on lines +1852 to +1862
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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}



class TableView(View):
Expand All @@ -1871,18 +1875,27 @@ def __init__(self, **kwargs):
self.columns = kwargs.get("columns", [])
self.row_actions = kwargs.get("row_actions", [])
self.tooltips = kwargs.get("tooltips", [])
self._tooltip_map = {}

def keys(self):
return [column.key for column in self.columns]

def add_column(self, key, **kwargs):
for column in self.columns:
if column.key == key:
raise ValueError(f"Column with key '{key}' already exists")

column = Column(key, **kwargs)
self.columns.append(column)
return column

def add_row_action(
self, name, on_click, label=None, icon=None, tooltip=None, **kwargs
):
for action in self.row_actions:
if action.name == name:
raise ValueError(f"Action with name '{name}' already exists")

row_action = Action(
name=name,
on_click=on_click,
Expand All @@ -1893,17 +1906,27 @@ def add_row_action(
)
self.row_actions.append(row_action)
return row_action

def add_tooltip(self, row, column, value, **kwargs):
if (row, column) in self._tooltip_map:
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_map[(row, column)] = tooltip
return tooltip

def clone(self):
clone = super().clone()
clone.columns = [column.clone() for column in self.columns]
clone.row_actions = [action.clone() for action in self.row_actions]
clone.tooltips = [tooltip.clone() for tooltip in self.tooltips]
clone._tooltip_map = {
(tooltip.row, tooltip.column): tooltip
for tooltip in clone.tooltips
}
return clone

def to_json(self):
Expand Down
46 changes: 46 additions & 0 deletions tests/unittests/operators/tableview_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import unittest

from fiftyone.operators.types import TableView


class TableViewTests(unittest.TestCase):
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"]

with self.assertRaises(ValueError):
table.add_column("column1", label="Column 3")

mock_on_click = lambda: None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)


table.add_row_action(
"action1",
on_click=mock_on_click,
icon="icon1",
color="primary",
tooltip="Action 1",
)
table.add_row_action(
"action2",
on_click=mock_on_click,
icon="icon2",
color="secondary",
tooltip="Action 2",
)

with self.assertRaises(ValueError):
table.add_row_action(
"action1",
on_click=mock_on_click,
icon="icon3",
color="primary",
tooltip="Action 3",
)

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")
Loading