Audit of `tango/utils.py` turned up a set of correctness bugs, some dead code, and a few duplication clusters. Every "wrong code" item below was reproduced at runtime, not only read. Line numbers refer to `tango/utils.py` as of `develop`. ## Bugs ### 1. `_seqStr_2_obj_from_type_format` passes its arguments in the wrong order `_seqStr_2_obj_from_type` is defined as `(seq, tg_type)` (line 1163), but all three call sites inside `_seqStr_2_obj_from_type_format` call it as `(tg_type, seq)`: - line 1203 — SCALAR branch - line 1205 — SPECTRUM branch - line 1219 — fallback / UNKNOWN_FORMAT With the operands swapped, none of the `tg_type in _scalar_*` / `_array_*` membership tests can match (a `list` is compared against a tuple of enum members), so the function falls through to `return []` at line 1198. It does not raise — it silently yields an empty result: ```python seqStr_2_obj(['1','2'], DevLong, AttrDataFormat.SPECTRUM) # -> [] expected [1, 2] seqStr_2_obj(['a','b'], DevString, AttrDataFormat.SPECTRUM) # -> [] expected ['a','b'] seqStr_2_obj(['true'], DevBoolean, AttrDataFormat.SPECTRUM) # -> [] expected [True] seqStr_2_obj([['true']], DevBoolean, AttrDataFormat.IMAGE) # -> [] expected [[True]] ``` This matters because `seqStr_2_obj` is public API: it is in `tango.utils.__all__`, re-exported from `tango/__init__.py`, and documented in `doc/api/utilities.md`. Two accidents mask the damage, and both need to be understood before fixing: 1. **The SCALAR branch is unreachable.** `seqStr_2_obj` gates on `if tg_format:` (line 1158), and `AttrDataFormat.SCALAR == 0`, so a SCALAR format is falsy and takes the *correct* no-format path instead. Lines 1202–1203 are therefore dead. The truthiness test on an enum whose first member is `0` is itself a latent bug — it should be `if tg_format is not None:` — and fixing that in isolation would **expose** the swapped call rather than fix it. The two need to be fixed together. 2. **The IMAGE branch partly works by accident**, because lines 1211–1216 inline the conversion using the correctly-named `seq`. Only an IMAGE of a type that is neither `DevString` nor scalar int/float (e.g. `DevBoolean`) falls through to the swapped line 1219. Also in the same function, line 1205 indexes `_scalar_to_array_type[tg_type]` unguarded, so a SPECTRUM request for a type absent from that map raises a bare `KeyError` instead of a useful message: ```python seqStr_2_obj(['x'], DevEnum, AttrDataFormat.SPECTRUM) # -> KeyError: <CmdArgType.DevEnum: 29> ``` The only in-tree caller (`tango/device_class.py`) never passes `tg_format`, which is why nothing in the test suite catches any of this. ### 2. `CaselessList.index()` raises `IndexError` instead of `ValueError`, and scans one element too far Lines 1495–1514. `minindex = max(0, minindex) - 1`, and the loop increments *before* indexing, so the final iteration reads `self[maxindex]`: ```python CaselessList(['a','b']).index('zzz') # -> IndexError expected ValueError CaselessList(['a','b']).index('b', 0, 1) # -> 1 expected ValueError (j=1 excludes index 1) ``` The method's own docstring states the contract as `s.index(x[, i[, j]])` returning the smallest `k` with `s[k] == x` and `i <= k < j`. Both the exception type and the half-open range are violated. The trailing `raise ValueError` is unreachable whenever `maxindex == len(self)`, i.e. in the default case. ### 3. `CaselessList.__iadd__` returns `None` Lines 1576–1579. It appends in place but never returns `self`, so Python rebinds the name to `None`: ```python l = CaselessList(['a']) l += ['b'] # l is None ``` ### 4. `CaselessList.__radd__` concatenates in the wrong order Lines 1571–1574 call `list.__add__(self, item)`, which is `self + item`. For the reflected operand the result must be `item + self`: ```python ['x'] + CaselessList(['a']) # -> ['a', 'x'] expected ['x', 'a'] ``` ### 5. `is_devstate_seq([])` crashes where `is_enum_seq([])` returns `False` `is_enum_seq` (line 774) guards its unwrapping loop with `and len(obj)`; `is_devstate_seq` (line 762) does not, so it indexes an empty sequence: ```python is_enum_seq([]) # -> False is_devstate_seq([]) # -> IndexError: list index out of range ``` `get_attribute_type_format` calls `is_devstate_seq(dtype)` *before* the code that produces the friendly `"Spectrum attribute type must be specified as (<dtype>,)"` message, so an empty `dtype` surfaces as a raw `IndexError` instead of that message. The two helpers should be symmetric. ### 6. `is_binary` ignores its documented `inc_array` parameter Line 964. The signature and docstring both advertise `inc_array`, but the body ignores it, and `_binary_types` (line 197) already contains the array type `DevVarCharArray` unconditionally. So `is_binary_type(DevVarCharArray)` is `True` even with the default `inc_array=False`, while every sibling predicate (`is_int`, `is_float`, `is_bool`, `is_str`, `is_numerical`) gates array types on that flag. `is_binary_type` is documented public API. Either honour the flag or drop the parameter and its docstring line. Honouring it is what the docstring promises, but it is a behaviour change for callers. ### 7. Stray debug `print` in `get_attrs` Line 1901 prints `type(obj.__dict__), obj` to stdout immediately before raising `TypeError`. Library code should not write to stdout; those values belong in the exception message. ### 8. `EventCallback._get_value` falls off the end for unrecognised event types Line 1761. The `isinstance` chain covers `EventData`, `AttrConfEventData`, `DataReadyEventData` and `DevIntrChangeEventData`; anything else returns `None` implicitly, and the callback then prints the literal string `None` as the event value. ### 9. `EventCallback._append` can raise when `max_buf=0` Line 1748: `if len(evts) == self._max_buf: evts.pop(0)`. With `max_buf=0` the first call pops from an empty list. Using `>=` fixes it and is also more robust if the buffer ever overshoots. ### 10. Mis-indented inner loop in `__build_to_tango_type` Lines 255–260. The `for key, value in FROM_TANGO_TO_NUMPY_TYPE.items():` loop sits *inside* the `for key in dir(CmdArgType):` loop and reuses both loop variables. The resulting dict is correct, but the inner body runs `len(dir(CmdArgType)) × len(FROM_TANGO_TO_NUMPY_TYPE)` times instead of once at import. Ruff flags it as `PLW2901` ("outer loop variable `key` overwritten by inner loop target"), which is the signature of an unintended indentation level. Dedenting it out of the outer loop is the whole fix. ## Dead code - **`copy_doc`** (line 1377) — no reference anywhere in the tree; not in `__all__`, not documented. Definition only. - **`_get_env_var`** (line 1816) — definition only. It reads `~/.tangorc` / `/etc/tangorc`, which nothing else uses. If that fallback is still wanted it has no caller to prove it works; if not, it can go. - **`_seqStr_2_obj_from_type_format` SCALAR branch** (lines 1202–1203) — unreachable, see bug 1. - **Python 2 relics**: `CaselessList.__setslice__` (line 1544) and `CaselessList.__getslice__` (line 1551) are never invoked under Python 3 — calling the latter raises `AttributeError: type object 'list' has no attribute '__getslice__'`. `CaselessDict.has_key` (line 1615) is the Python 2 spelling of `in`. The `CaselessList` class docstring still advertises the two slice methods as implemented. - **`is_pure_str` docstring** still documents Python 2 behaviour ("In python 2.x this means any subclass of basestring"). - **20 no-op `global` statements** across the `is_*` predicates and the `_seqStr_2_obj_*` helpers (Ruff `PLW0602`). `global` is only needed to rebind a module-level name; all of these merely read. - The derived-type loop after `_scalar_to_array_type` leaves `k` and `v` bound as module-level globals in `tango.utils`. ## Duplication - **Four near-identical sequence↔vector converter pairs**: `seq_2_StdStringVector` / `StdStringVector_2_seq`, `seq_2_StdDoubleVector` / `StdDoubleVector_2_seq`, `seq_2_DbDevInfos`, `seq_2_DbDevExportInfos`. All share one body shape and differ only in the container class and the per-element coercion. `seq_2_DbDevInfos` and `seq_2_DbDevExportInfos` are literally identical except for the class name in three places. A parameterised helper plus thin wrappers would remove roughly 90 lines. - **Five near-identical type predicates**: `is_numerical`, `is_int`, `is_float`, `is_bool`, `is_str` are the same six lines over different module-level tuples. `is_binary` *looks* like a sixth but silently diverges — which is exactly how bug 6 went unnoticed. Generating them from a `(scalar_types, array_types)` table would make that divergence impossible. - `document_method` / `document_static_method` differ only in whether the doc is written to the bound method or the function, plus a `__name__` fix-up only the former does. - `dir2` calls both `get_attrs(cls)` and `dir2(cls)` on each base, but `dir2(cls)` already includes `get_attrs(cls)`, so the first call is pure extra work on every recursive step. The result is a `set`, so correctness is unaffected. - `doc/api/utilities.md` documents `is_bool` and `is_bool_type`, which are the same function object under two names. ## Why these survived `tango/utils.py` sits at **66%** statement coverage (branch coverage enabled), against roughly 77% for the package as a whole. The uncovered regions line up almost exactly with the bug list — per-symbol line coverage, worst first: | symbol | coverage | related item | |---|---|---| | `_get_env_var` | 5% | dead | | `_seqStr_2_obj_from_type_format` | 8% | bug 1 | | `document_method` | 9% | duplication | | `document_static_method` | 12% | duplication | | `CaselessList` | 22% | bugs 2, 3, 4 | | `copy_doc` | 25% | dead | | `CaselessDict` | 38% | dead relic | | `is_binary` | 50% | bug 6 | | `get_attrs` | 62% | bug 7 | | `EventCallback` | 72% | bugs 8, 9 | Lines 1202–1219 — the whole swapped-argument block — are never executed by any test. None of `CaselessList`, `CaselessDict`, `seqStr_2_obj`, `is_binary`, `is_devstate_seq`, `dir2`, `strtobool`, `copy_doc` or `_get_env_var` is referenced by a single test. One caveat on relying on coverage here: `is_devstate_seq` reports 100% *line* coverage yet still crashes on `[]`, because the empty-sequence input shares a line with the guarded path. That one needs its own test case rather than a coverage target. ## Suggested order 1. Bug 1 together with the `if tg_format:` truthiness bug — public, documented, silently wrong. Regression test first (SPECTRUM round-trip), then the fix. 2. Bugs 2–4 (`CaselessList`) — small, self-contained, no in-tree callers, so low risk. 3. Bugs 5, 7, 8, 9, 10 — small correctness and robustness fixes. 4. Bug 6 — needs a decision on whether to honour or drop `inc_array`. 5. Dead-code removal and the Python 2 relics. 6. The duplication clusters, which is what stops a bug-6-style divergence recurring.