Merge pull request #4257 from jenshnielsen/validate_full_name

Let instrument names be less restrictive.

docs/changes/newsfragments/4257.breaking

@@ -0,0 +1,2 @@
+Restrictions on instrument names are now less strict than in ``0.34.0``. Submodules are allowed
+to have names that are not valid identifier as long as the full name is an valid identifier.

qcodes/instrument/base.py

@@ -52,8 +52,9 @@ class InstrumentBase(Metadatable, DelegateAttributes):
     """
 
     def __init__(self, name: str, metadata: Optional[Mapping[Any, Any]] = None) -> None:
-        name = self._is_valid_identifier(name)
-        self._short_name = str(name)
+        name = self._replace_hyphen(name)
+        self._short_name = name
+        self._is_valid_identifier(self.full_name)
 
         self.parameters: Dict[str, _BaseParameter] = {}
         """
@@ -409,13 +410,17 @@ def short_name(self) -> str:
         return self._short_name
 
     @staticmethod
-    def _is_valid_identifier(name: str) -> str:
+    def _is_valid_identifier(name: str) -> None:
         """Check whether given name is a valid instrument identifier."""
-        new_name = name.replace("-", "_")
+        if not name.isidentifier():
+            raise ValueError(f"{name} invalid instrument identifier")
+
+    @staticmethod
+    def _replace_hyphen(name: str) -> str:
+        """Replace - in name with _ and warn if any is found."""
+        new_name = str(name).replace("-", "_")
         if name != new_name:
             warnings.warn(f"Changed {name} to {new_name} for instrument identifier")
-        if not new_name.isidentifier():
-            raise ValueError(f"{new_name} invalid instrument identifier")
 
         return new_name
 

qcodes/instrument/channel.py

@@ -89,7 +89,7 @@ def root_instrument(self) -> InstrumentBase:
 
     @property
     def name_parts(self) -> List[str]:
-        name_parts = self._parent.name_parts
+        name_parts = list(self._parent.name_parts)
         name_parts.append(self.short_name)
         return name_parts
 

qcodes/tests/drivers/keysight_b1500/b1500_driver_tests/conftest.py

@@ -1,4 +1,4 @@
-from unittest.mock import MagicMock
+from unittest.mock import MagicMock, PropertyMock
 
 import pytest
 from pyvisa import VisaIOError
@@ -34,4 +34,12 @@ def _make_b1500(request):
 
 @pytest.fixture(name="mainframe")
 def _make_mainframe():
-    yield MagicMock()
+    PropertyMock()
+    mainframe = MagicMock()
+    name_parts = PropertyMock(return_value=["mainframe"])
+    type(mainframe).name_parts = name_parts
+    short_name = PropertyMock(return_value="short_name")
+    type(mainframe).short_name = short_name
+    full_name = PropertyMock(return_value="mainframe")
+    type(mainframe).full_name = full_name
+    yield mainframe

qcodes/tests/instrument_mocks.py

@@ -424,14 +424,20 @@ class DummyChannelInstrument(Instrument):
     Dummy instrument with channels
     """
 
-    def __init__(self, name, **kwargs):
+    def __init__(self, name, channel_names=None, **kwargs):
         super().__init__(name, **kwargs)
 
         channels = ChannelList(self, "TempSensors", DummyChannel, snapshotable=False)
-        for chan_name in ('A', 'B', 'C', 'D', 'E', 'F'):
-            channel = DummyChannel(self, f'Chan{chan_name}', chan_name)
+
+        if channel_names is None:
+            channel_ids = ("A", "B", "C", "D", "E", "F")
+            channel_names = tuple(f"Chan{chan_name}" for chan_name in channel_ids)
+        else:
+            channel_ids = channel_names
+        for chan_name, chan_id in zip(channel_names, channel_ids):
+            channel = DummyChannel(self, chan_name, chan_id)
             channels.append(channel)
-            self.add_submodule(chan_name, channel)
+            self.add_submodule(chan_id, channel)
         self.add_submodule("channels", channels.to_channel_tuple())
 
 

qcodes/tests/test_instrument.py

@@ -96,6 +96,8 @@ def test_instrument_on_invalid_identifier(close_before_and_after):
     assert DummyInstrument.instances() == []
     assert Instrument._all_instruments == {}
 
+
+def test_instrument_warns_on_hyphen_in_name(close_before_and_after):
     # Check if warning is raised and name is valid
     # identifier when dashes '-' are converted to underscores '_'
     with pytest.warns(
@@ -109,6 +111,25 @@ def test_instrument_on_invalid_identifier(close_before_and_after):
     assert Instrument._all_instruments != {}
 
 
+def test_instrument_allows_channel_name_starting_with_number(close_before_and_after):
+    instr = DummyChannelInstrument(name="foo", channel_names=["1", "2", "3"])
+
+    for chan in instr.channels:
+        assert chan.short_name.isidentifier() is False
+        assert chan.full_name.isidentifier() is True
+    assert Instrument.instances() == []
+    assert DummyChannelInstrument.instances() == [instr]
+    assert Instrument._all_instruments != {}
+
+
+def test_instrument_channel_name_raise_on_invalid(close_before_and_after):
+    with pytest.raises(ValueError, match="foo_☃ invalid instrument identifier"):
+        DummyChannelInstrument(name="foo", channel_names=["☃"])
+    assert Instrument.instances() == []
+    assert DummyChannelInstrument.instances() == []
+    assert Instrument._all_instruments == {}
+
+
 def test_instrument_retry_with_same_name(close_before_and_after):
     with pytest.raises(RuntimeError, match="Failed to create instrument"):
         DummyFailingInstrument(name="failinginstrument")