Address Copilot review comments

- Use explicit None checks instead of or-chaining for URDF input selection
- Improve error message specificity in RobotModelFromURDF
- Update docstring to explain auto-detection behavior (XML vs file path)
- Add XML content detection (<?xml or <robot prefix) before file path check
- Standardize R8_6 API to match other robots (remove urdf_path parameter)
- Add test coverage for new URDF parameters and deprecation warning

Author: iory

skrobot/model/robot_model.py

@@ -1847,9 +1847,13 @@ def __init__(self, link_list=None, joint_list=None,
         root_link : Link, optional
             Root link of the robot model.
         urdf : str, optional
-            URDF input used to build the robot model. This can be either a
-            file system path to a URDF file or a string containing URDF XML.
-            If None (the default), the model is not initialized from URDF.
+            URDF input used to build the robot model. The value is interpreted
+            automatically: if the input looks like XML content (starts with
+            '<?xml' or '<robot'), it is treated as a string containing URDF
+            XML; otherwise, if ``os.path.isfile`` returns True, it is treated
+            as a file system path to a URDF file; otherwise it is treated as
+            URDF XML string. If None (the default), the model is not
+            initialized from URDF.
         include_mimic_joints : bool, optional (default: True)
             Whether to include mimic joints defined in the URDF when loading
             the model.
@@ -1880,6 +1884,9 @@ def _load_from_urdf(self, urdf_input, include_mimic_joints=True):
         """Load URDF from a string or a file path.
 
         Automatically detects if the input is a URDF string or a file path.
+        If the input looks like XML content (starts with '<?xml' or '<robot'),
+        it is treated as URDF XML string. Otherwise, if the input is an
+        existing file path, it is loaded as a file.
 
         Parameters
         ----------
@@ -1890,6 +1897,15 @@ def _load_from_urdf(self, urdf_input, include_mimic_joints=True):
             If True, mimic joints are included in the resulting
             `RobotModel`'s `joint_list`.
         """
+        # First, check if the input looks like URDF/XML content
+        if isinstance(urdf_input, str):
+            stripped = urdf_input.lstrip()
+            if stripped.startswith('<?xml') or stripped.startswith('<robot'):
+                self.load_urdf(
+                    urdf_input, include_mimic_joints=include_mimic_joints)
+                return
+
+        # If it doesn't look like XML, check if it's a file path
         if os.path.isfile(urdf_input):
             with open(urdf_input, 'r') as f:
                 self.load_urdf_file(

skrobot/models/fetch.py

@@ -18,7 +18,12 @@ def __init__(self, urdf=None, urdf_file=None):
             raise ValueError(
                 "'urdf' and 'urdf_file' cannot be given at the same time"
             )
-        urdf_input = urdf or urdf_file or fetch_urdfpath()
+        if urdf is not None:
+            urdf_input = urdf
+        elif urdf_file is not None:
+            urdf_input = urdf_file
+        else:
+            urdf_input = fetch_urdfpath()
         super(Fetch, self).__init__(urdf=urdf_input)
         self.rarm_end_coords = CascadedCoords(parent=self.gripper_link,
                                               name='rarm_end_coords')

skrobot/models/kuka.py

@@ -15,7 +15,12 @@ def __init__(self, urdf=None, urdf_file=None):
             raise ValueError(
                 "'urdf' and 'urdf_file' cannot be given at the same time"
             )
-        urdf_input = urdf or urdf_file or kuka_urdfpath()
+        if urdf is not None:
+            urdf_input = urdf
+        elif urdf_file is not None:
+            urdf_input = urdf_file
+        else:
+            urdf_input = kuka_urdfpath()
         super(Kuka, self).__init__(urdf=urdf_input)
         self.rarm_end_coords = CascadedCoords(
             parent=self.lbr_iiwa_with_wsg50__lbr_iiwa_link_7,

skrobot/models/nextage.py

@@ -23,7 +23,12 @@ def __init__(self, urdf=None, urdf_file=None):
             raise ValueError(
                 "'urdf' and 'urdf_file' cannot be given at the same time"
             )
-        urdf_input = urdf or urdf_file or nextage_urdfpath()
+        if urdf is not None:
+            urdf_input = urdf
+        elif urdf_file is not None:
+            urdf_input = urdf_file
+        else:
+            urdf_input = nextage_urdfpath()
         super(Nextage, self).__init__(urdf=urdf_input)
 
         # End effector coordinates

skrobot/models/panda.py

@@ -19,7 +19,12 @@ def __init__(self, urdf=None, urdf_file=None):
             raise ValueError(
                 "'urdf' and 'urdf_file' cannot be given at the same time"
             )
-        urdf_input = urdf or urdf_file or panda_urdfpath()
+        if urdf is not None:
+            urdf_input = urdf
+        elif urdf_file is not None:
+            urdf_input = urdf_file
+        else:
+            urdf_input = panda_urdfpath()
         super(Panda, self).__init__(urdf=urdf_input)
 
         # End effector coordinate frame

skrobot/models/pr2.py

@@ -18,7 +18,12 @@ def __init__(self, urdf=None, urdf_file=None, use_tight_joint_limit=True):
             raise ValueError(
                 "'urdf' and 'urdf_file' cannot be given at the same time"
             )
-        urdf_input = urdf or urdf_file or pr2_urdfpath()
+        if urdf is not None:
+            urdf_input = urdf
+        elif urdf_file is not None:
+            urdf_input = urdf_file
+        else:
+            urdf_input = pr2_urdfpath()
         super(PR2, self).__init__(urdf=urdf_input)
 
         self.rarm_end_coords = CascadedCoords(

skrobot/models/r8_6.py

@@ -5,17 +5,18 @@
 
 class R8_6(skrobot.model.RobotModel):
 
-    def __init__(self, urdf=None, urdf_file=None, urdf_path=None):
-        # For backward compatibility, support urdf, urdf_file, and urdf_path
-        # urdf_path is treated as an alias for urdf_file for backward compatibility
-        specified_sources = [
-            src for src in (urdf, urdf_file, urdf_path) if src is not None
-        ]
-        if len(specified_sources) > 1:
+    def __init__(self, urdf=None, urdf_file=None):
+        # For backward compatibility, support both urdf and urdf_file
+        if urdf is not None and urdf_file is not None:
             raise ValueError(
-                "Only one of 'urdf', 'urdf_file', or 'urdf_path' can be specified"
+                "'urdf' and 'urdf_file' cannot be given at the same time"
             )
-        urdf_input = urdf or urdf_file or urdf_path or r8_6_urdfpath()
+        if urdf is not None:
+            urdf_input = urdf
+        elif urdf_file is not None:
+            urdf_input = urdf_file
+        else:
+            urdf_input = r8_6_urdfpath()
         super(R8_6, self).__init__(urdf=urdf_input)
 
         # Define end effector coordinates for left arm

skrobot/models/urdf.py

@@ -20,7 +20,8 @@ def __init__(self, urdf=None, urdf_file=None):
         )
         if urdf is not None and urdf_file is not None:
             raise ValueError(
-                "'urdf' and 'urdf_file' cannot be given at the same time"
+                "'urdf' and 'urdf_file' cannot be given at the same time; "
+                "received both 'urdf' and 'urdf_file' arguments."
             )
         if urdf is not None:
             urdf_input = urdf

tests/skrobot_tests/test_urdf.py

@@ -1,8 +1,13 @@
 import os
 import sys
 import unittest
+import warnings
 
 from skrobot.data import fetch_urdfpath
+from skrobot.data import panda_urdfpath
+from skrobot.model import RobotModel
+from skrobot.models import Fetch
+from skrobot.models import Panda
 from skrobot.models.urdf import RobotModelFromURDF
 from skrobot.utils.urdf import mesh_simplify_factor
 
@@ -32,3 +37,62 @@ def test_load_urdfmodel_with_simplification(self):
         # load using existing cache
         with mesh_simplify_factor(0.1):
             RobotModelFromURDF(urdf_file=fetch_urdfpath())
+
+
+class TestRobotModelFromURDFDeprecation(unittest.TestCase):
+
+    def test_deprecation_warning(self):
+        """Test that RobotModelFromURDF raises DeprecationWarning."""
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter("always")
+            RobotModelFromURDF(urdf_file=panda_urdfpath())
+            self.assertEqual(len(w), 1)
+            self.assertTrue(issubclass(w[0].category, DeprecationWarning))
+            self.assertIn("deprecated", str(w[0].message).lower())
+
+
+class TestRobotModelURDFParameters(unittest.TestCase):
+
+    def test_robot_model_with_urdf_file_path(self):
+        """Test RobotModel initialization with urdf file path."""
+        robot = RobotModel(urdf=panda_urdfpath())
+        self.assertEqual(robot.name, "panda")
+
+    def test_robot_model_with_urdf_string(self):
+        """Test RobotModel initialization with urdf string."""
+        with open(panda_urdfpath(), 'r') as f:
+            urdf_string = f.read()
+        robot = RobotModel(urdf=urdf_string)
+        self.assertEqual(robot.name, "panda")
+
+    def test_robot_model_from_urdf_static_method(self):
+        """Test RobotModel.from_urdf static method."""
+        robot = RobotModel.from_urdf(panda_urdfpath())
+        self.assertEqual(robot.name, "panda")
+
+    def test_panda_with_custom_urdf(self):
+        """Test Panda with custom urdf parameter."""
+        panda = Panda(urdf=panda_urdfpath())
+        self.assertEqual(panda.name, "panda")
+
+    def test_panda_with_custom_urdf_file(self):
+        """Test Panda with custom urdf_file parameter."""
+        panda = Panda(urdf_file=panda_urdfpath())
+        self.assertEqual(panda.name, "panda")
+
+    def test_fetch_with_custom_urdf(self):
+        """Test Fetch with custom urdf parameter."""
+        fetch = Fetch(urdf=fetch_urdfpath())
+        self.assertEqual(fetch.name, "fetch")
+
+    def test_panda_raises_error_with_both_urdf_params(self):
+        """Test that providing both urdf and urdf_file raises ValueError."""
+        with self.assertRaises(ValueError) as context:
+            Panda(urdf="test", urdf_file="test2")
+        self.assertIn("cannot be given at the same time", str(context.exception))
+
+    def test_fetch_raises_error_with_both_urdf_params(self):
+        """Test that providing both urdf and urdf_file raises ValueError."""
+        with self.assertRaises(ValueError) as context:
+            Fetch(urdf="test", urdf_file="test2")
+        self.assertIn("cannot be given at the same time", str(context.exception))