Repository: Textualize/textual

Issue:
Make moving a child before or after itself, a no-op
If we try to use `move_child` to move a widget to after/before itself, should we get an error or is that a no-op?
(Add tests to tests/test_widget_child_moving.py accordingly.)

Context: this arose from writing some functionality in a TODO app that sorts TODO items according to their due date and reorders them when the due date is edited. From this point of view, it makes sense for moving to after/before itself to be a no-op.
Otherwise, the sorting code needs to take extra steps to make sure we don't try to move things to where they already are.

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5303ec554..177202444 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+
+### Fixed
+
+- `Widget.move_child` would break if `before`/`after` is set to the index of the widget in `child` https://github.com/Textualize/textual/issues/1743
+
+### Changed
+
+- Breaking change: `Widget.move_child` parameters `before` and `after` are now keyword-only https://github.com/Textualize/textual/pull/3896
+
 ## [0.46.0] - 2023-12-17
 
 ### Fixed
diff --git a/src/textual/widget.py b/src/textual/widget.py
index 4de0709a3..a4fa033e1 100644
--- a/src/textual/widget.py
+++ b/src/textual/widget.py
@@ -892,9 +892,30 @@ class Widget(DOMNode):
         await_mount = self.mount(*widgets, before=before, after=after)
         return await_mount
 
+    @overload
+    def move_child(
+        self,
+        child: int | Widget,
+        *,
+        before: int | Widget,
+        after: None = None,
+    ) -> None:
+        ...
+
+    @overload
+    def move_child(
+        self,
+        child: int | Widget,
+        *,
+        after: int | Widget,
+        before: None = None,
+    ) -> None:
+        ...
+
     def move_child(
         self,
         child: int | Widget,
+        *,
         before: int | Widget | None = None,
         after: int | Widget | None = None,
     ) -> None:
@@ -920,10 +941,6 @@ class Widget(DOMNode):
         elif before is not None and after is not None:
             raise WidgetError("Only one of `before` or `after` can be handled.")
 
-        # We short-circuit the no-op, otherwise it will error later down the road.
-        if child is before or child is after:
-            return
-
         def _to_widget(child: int | Widget, called: str) -> Widget:
             """Ensure a given child reference is a Widget."""
             if isinstance(child, int):
@@ -948,6 +965,9 @@ class Widget(DOMNode):
             cast("int | Widget", before if after is None else after), "move towards"
         )
 
+        if child is target:
+            return  # Nothing to be done.
+
         # At this point we should know what we're moving, and it should be a
         # child; where we're moving it to, which should be within the child
         # list; and how we're supposed to move it. All that's left is doing


Test patch:
diff --git a/tests/test_widget_child_moving.py b/tests/test_widget_child_moving.py
index f2d40a4aa..f0ab58d30 100644
--- a/tests/test_widget_child_moving.py
+++ b/tests/test_widget_child_moving.py
@@ -9,7 +9,7 @@ from textual.widget import Widget, WidgetError
 async def test_move_child_no_direction() -> None:
     """Test moving a widget in a child list."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(child)
@@ -18,7 +18,7 @@ async def test_move_child_no_direction() -> None:
 async def test_move_child_both_directions() -> None:
     """Test calling move_child with more than one direction."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(child, before=1, after=2)
@@ -27,7 +27,7 @@ async def test_move_child_both_directions() -> None:
 async def test_move_child_not_our_child() -> None:
     """Test attempting to move a child that isn't ours."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(Widget(), before=child)
@@ -36,28 +36,82 @@ async def test_move_child_not_our_child() -> None:
 async def test_move_child_to_outside() -> None:
     """Test attempting to move relative to a widget that isn't a child."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(child, before=Widget())
 
 
-async def test_move_child_before_itself() -> None:
-    """Test moving a widget before itself."""
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_index_in_relation_to_itself_index(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
 
+    widget = Widget()
+    child = 0
+    kwargs = {reference: 0}
     async with App().run_test() as pilot:
-        child = Widget(Widget())
-        await pilot.app.mount(child)
-        pilot.app.screen.move_child(child, before=child)
-
-
-async def test_move_child_after_itself() -> None:
-    """Test moving a widget after itself."""
-    # Regression test for https://github.com/Textualize/textual/issues/1743
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
+
+
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_index_in_relation_to_itself_widget(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
+
+    widget = Widget()
+    child = 0
+    kwargs = {reference: widget}
     async with App().run_test() as pilot:
-        child = Widget(Widget())
-        await pilot.app.mount(child)
-        pilot.app.screen.move_child(child, after=child)
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
+
+
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_widget_in_relation_to_itself_index(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
+
+    widget = Widget()
+    child = widget
+    kwargs = {reference: 0}
+    async with App().run_test() as pilot:
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
+
+
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_widget_in_relation_to_itself_widget(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
+
+    widget = Widget()
+    child = widget
+    kwargs = {reference: widget}
+    async with App().run_test() as pilot:
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
 
 
 async def test_move_past_end_of_child_list() -> None: