From 17bf959e9348e2fc8e847ddf35ee58ca8ca81aaf Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 5 Jul 2026 16:21:56 +1000 Subject: [PATCH] fix(models): rename list()'s type= param to mtype=, matching stale docs The runblock examples in arm_dh.rst/arm_erobot.rst called rtb.models.list(mtype=...), but the actual parameter was named type=, shadowing the builtin. Renaming the parameter to mtype (rather than fixing the docs to say type=) avoids the builtin shadow and matches what the docs already expected. list() the function still shadows builtin list; logged as tech debt since renaming public API needs a breaking-change pass. Co-Authored-By: Claude Sonnet 5 --- src/roboticstoolbox/models/list.py | 12 +++++------ tech-debt.md | 33 ++++++++++++++++++++++++++++++ tests/test_models.py | 2 +- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/roboticstoolbox/models/list.py b/src/roboticstoolbox/models/list.py index 94cb31b85..16ff58cfa 100644 --- a/src/roboticstoolbox/models/list.py +++ b/src/roboticstoolbox/models/list.py @@ -8,7 +8,7 @@ # import importlib -def list(keywords=None, dof=None, type=None, border="thin"): +def list(keywords=None, dof=None, mtype=None, border="thin"): """ Display all robot models in summary form @@ -16,13 +16,13 @@ def list(keywords=None, dof=None, type=None, border="thin"): :type keywords: tuple of str, optional :param dof: number of DoF to filter on, defaults to None :type dof: int, optional - :param type: model type "DH", "ETS", "URDF", defaults to all types - :type type: str, optional + :param mtype: model type "DH", "ETS", "URDF", defaults to all types + :type mtype: str, optional - ``list()`` displays a list of all models provided by the Toolbox. It lists the name, manufacturer, model type, number of DoF, and keywords. - - ``list(type=MT)`` as above, but only displays models of type ``MT`` + - ``list(mtype=MT)`` as above, but only displays models of type ``MT`` where ``MT`` is one of "DH", "ETS" or "URDF". - ``list(keywords=KW)`` as above, but only displays models that have a @@ -61,8 +61,8 @@ def make_table(border=None): border=border, ) - if type is not None: - categories = [type] + if mtype is not None: + categories = [mtype] else: categories = ["DH", "URDF", "ETS"] for category in categories: diff --git a/tech-debt.md b/tech-debt.md index f6ad93c41..368bdd12b 100644 --- a/tech-debt.md +++ b/tech-debt.md @@ -683,3 +683,36 @@ handles the shadowing correctly on its own. Also worth a quick sweep for any other `sys.version_info`/Python-3.10-specific conditionals elsewhere in the codebase at that point, so 3.10 cleanup happens in one pass rather than piecemeal. + +## `roboticstoolbox.models.list()` shadows the builtin `list` + +### Background + +Found 2026-07-05 while fixing a stale `mtype=` kwarg in the docs +(`arm_dh.rst`/`arm_erobot.rst` runblock examples called +`rtb.models.list(mtype="DH")`, but the function's parameter had been +renamed to `type` at some point without updating the docs). + +Two things stood out while looking at `src/roboticstoolbox/models/list.py`: + +1. The function itself is named `list`, shadowing the builtin `list` + within any scope that does `from roboticstoolbox.models.list import + list` or `import roboticstoolbox.models as models; models.list(...)`. + It's always called qualified (`rtb.models.list(...)`) in practice, so + this hasn't bitten anyone yet, but it's a landmine for future edits to + that file — reaching for `list(...)` to build an actual list inside the + function body would silently recurse/shadow instead of erroring. +2. (Already fixed, see commit around 2026-07-05) one of its parameters was + named `type`, shadowing the builtin `type`. This has been renamed to + `mtype` — matching what the docs already (mistakenly, but presciently) + assumed the parameter was called — and all call sites (docs, tests) + updated to match. + +### Proposed fix + +Renaming the function itself (e.g. to `list_models`) would fix the +remaining shadowing, but `list` is public API (`rtb.models.list`) and a +rename is a breaking change for any external callers — deferred rather +than done opportunistically. If/when a broader API-breaking pass happens +on this module (or at the next major version bump), rename `list` to +something that doesn't shadow a builtin. diff --git a/tests/test_models.py b/tests/test_models.py index f303b768a..29bd37464 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -13,7 +13,7 @@ class TestModels(unittest.TestCase): def test_list(self): rp.models.list() rp.models.list("UR", 6) - rp.models.list(type="DH") + rp.models.list(mtype="DH") def test_puma(self): puma = rp.models.DH.Puma560()