Repository: Textualize/textual
Pull request: Move child acts as noop if trying to move before/after itself
This fixes #1743 for good.

Problem statement:
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.

Hints from issue discussion:
The more I think about it, the more I think I'm inclined to agree with you that it makes sense to have it be a no-op. "Move child to where it is" seems like it shouldn't error even if it's a weird thing to ask, as it's not the same as calling `move_child` and not providing anywhere, or providing somewhere out of bounds. The request is a valid location and is within bounds.
No-op makes sense.
Note to self: fix the docstrings in `move_child` as well.
Note to self x2: Dave owes me a favour.
> Note to self x2: Dave owes me a favour.

Let's call it payback for retconning my first blog post. 😛
Does this require work? What happens now if you move a child before/after itself?

I agree that should be a no-op and not an error.
Just tested this and it appears to still be a problem. I'll turn these into proper `XFAIL` tests so we've got them noted.
And the problem is what, exactly? Moving a child before itself raises an error and we think it should be a no-op?
Yes and [yes](https://github.com/Textualize/textual/issues/1743#issuecomment-1422886317).
Don't forget to [star](https://github.com/Textualize/textual) the repository!

Follow [@textualizeio](https://twitter.com/textualizeio) for Textual updates.
This should also be a no-op if `before`/`after` are the indices of the widget we are trying to move.

From [this comment on Discord](https://discord.com/channels/1026214085173461072/1033754296224841768/1185063380294316133).

Commit message: Link to PR in changelog.

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5303ec554f..1772024442 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 4de0709a38..a4fa033e1c 100644
--- a/src/textual/widget.py
+++ b/src/textual/widget.py
@@ -892,9 +892,30 @@ def mount_all(
         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 @@ def move_child(
         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 @@ def _to_widget(child: int | Widget, called: str) -> Widget:
             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
diff --git a/tests/test_widget_child_moving.py b/tests/test_widget_child_moving.py
index f2d40a4aae..efb69e42e9 100644
--- a/tests/test_widget_child_moving.py
+++ b/tests/test_widget_child_moving.py
@@ -9,7 +9,7 @@
 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,66 @@ 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(
+    ["type_of_child", "before"],
+    [
+        ("widget", "widget"),
+        ("widget", "index"),
+        ("index", "widget"),
+        ("index", "index"),
+    ],
+)
+async def test_move_child_before_itself(type_of_child: str, before: str) -> None:
+    """Test moving a widget before itself.
+
+    Regression test for https://github.com/Textualize/textual/issues/1743
+    """
+
+    widget = Widget()
+    keys = {
+        "widget": widget,
+        "index": 0,
+    }
+    child = keys[type_of_child]
+    before = keys[before]
 
     async with App().run_test() as pilot:
-        child = Widget(Widget())
-        await pilot.app.mount(child)
-        pilot.app.screen.move_child(child, before=child)
-
+        await pilot.app.mount(widget)
+        pilot.app.screen.move_child(child, before=before)
+
+
+@pytest.mark.parametrize(
+    ["type_of_child", "after"],
+    [
+        ("widget", "widget"),
+        ("widget", "index"),
+        ("index", "widget"),
+        ("index", "index"),
+    ],
+)
+async def test_move_child_after_itself(type_of_child: str, after: str) -> None:
+    """Test moving a widget after itself.
+
+    Regression test for https://github.com/Textualize/textual/issues/1743
+    """
+
+    widget = Widget()
+    keys = {
+        "widget": widget,
+        "index": 0,
+    }
+    child = keys[type_of_child]
+    after = keys[after]
 
-async def test_move_child_after_itself() -> None:
-    """Test moving a widget after itself."""
-    # Regression test for https://github.com/Textualize/textual/issues/1743
     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.mount(widget)
+        pilot.app.screen.move_child(child, after=after)
 
 
 async def test_move_past_end_of_child_list() -> None:


Code review comments:
diff_hunk: @@ -36,28 +36,66 @@ 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(
+    ["type_of_child", "before"],
+    [
+        ("widget", "widget"),
+        ("widget", "index"),
+        ("index", "widget"),
+        ("index", "index"),
+    ],
+)
+async def test_move_child_before_itself(type_of_child: str, before: str) -> None:
+    """Test moving a widget before itself.
+
+    Regression test for https://github.com/Textualize/textual/issues/1743
+    """
+
+    widget = Widget()
+    keys = {
+        "widget": widget,
+        "index": 0,
+    }
+    child = keys[type_of_child]
+    before = keys[before]
 
     async with App().run_test() as pilot:
-        child = Widget(Widget())
-        await pilot.app.mount(child)
-        pilot.app.screen.move_child(child, before=child)
-
+        await pilot.app.mount(widget)
+        pilot.app.screen.move_child(child, before=before)
+
+
+@pytest.mark.parametrize(
+    ["type_of_child", "after"],
+    [
+        ("widget", "widget"),
+        ("widget", "index"),
+        ("index", "widget"),
+        ("index", "index"),
+    ],
+)
+async def test_move_child_after_itself(type_of_child: str, after: str) -> None:
+    """Test moving a widget after itself.
+
+    Regression test for https://github.com/Textualize/textual/issues/1743
+    """
+
+    widget = Widget()
+    keys = {
+        "widget": widget,
+        "index": 0,
+    }
+    child = keys[type_of_child]
+    after = keys[after]
path: tests/test_widget_child_moving.py
text: @user1:
Why the extra lookup here? Does something like the code below not work?

```python
[
  (Widget(), Widget()),
  (Widget(), 0),
  # ...
]
```

And just directly pass in `child` and `after`?

@author:
Your proposal will generate many different `Widget` instances and I need to check what happens when the child and before/after `is` the same instance, whereas if `child, after = Widget(), Widget()`, then we get `child is not after`.

@user1:
I see. The reason that I'm asking is that I find it hard to follow, and the use of conditionals and lookups (which are basically just conditionals) is usually considered a code smell in a parametrised test - it suggests the test is trying to do cover too many things, and there's almost always a better way. Usually, that means breaking the test up into multiple, more readable tests, even if it means duplication.

In this case, I think you're trying to shoehorn in a "a widget can be specified as a integer" test into this test, rather than covering it off in an isolated manner (a separate unit test). If you do this in a separate test, then you can let this test focus on one unit of functionality: move_child nooping.

One other thing is - what is this intended to test? It just calls `move_child` then does nothing. The title of the PR explains that it should no-op, so we could check (within reason) that that's the actual behaviour.

Maybe something like this, to ensure the DOM isn't modified and that it is in fact a null operation?

```python
screen = pilot.app.screen
await pilot.app.mount(widget)
dom_before = screen.walk_children()

screen.move_child(widget, after=widget)

dom_after = screen.walk_children()
assert dom_before == dom_after
```


@author:
Point taken regarding lookups/conditionals and parametrised tests.

Regarding the testing, I'm testing that the function call doesn't raise an error (before the PR, it did).
I can be extra safe and check that the DOM did stay the same, but given that the screen has a single `Widget` child, doesn't make a lot of sense to me.

@author:
Thanks for your feedback. I've addressed it in e03de27.