Prevent non str keys #3247
Prevent non str keys #3247
Conversation
src/scipp/core/data_group.py
Outdated
| if isinstance(name, str): | ||
| self._items[name] = value | ||
| else: | ||
| raise ValueError( |
There was a problem hiding this comment.
| raise ValueError( | |
| raise TypeError( |
src/scipp/core/data_group.py
Outdated
| self._items[name] = value | ||
| else: | ||
| raise ValueError( | ||
| 'Assigning to slice with implicit dimension label is not possible.' |
There was a problem hiding this comment.
Why 'implicit dim labels'? I would expect the message to talk about the type.
There was a problem hiding this comment.
Ah, I read the example in the issue again. Maybe the message should be a little longer and be more explicit about types and slicing.
There was a problem hiding this comment.
Yes I agree the error message is a bit confusing, because, with the implementation in this PR, assigning to a slice with an explicit dimension label is also not possible ;)
Maybe it would be better to have something simpler like KeyError: non 'str' keys not allowed in datagroup. Did you mean to assign to one of the underlying variables in the datagroup?.
There was a problem hiding this comment.
How about simply: "Keys must be strings"?
There was a problem hiding this comment.
It shouldn't be KeyError because that is used for failed lookups. But otherwise, I'm fine with either message
There was a problem hiding this comment.
Okay I'll make it simply "TypeError: Keys must be strings"
There was a problem hiding this comment.
The slightly confusing thing from an api perspective is that you can get items using slicing without specifying the label:
import scipp as sc
ds = sc.DataGroup(a=sc.arange('x', 4), b=sc.arange('y', 6))
ds['x', 2] # This works, returns a DataGroupbut if you try to set something without specifying the label
ds['x', 2] = sc.scalar(6) it raises TypeError: Keys must be strings.
What they should do of course is:
ds['a']['x', 2] = sc.scalar(6)There was a problem hiding this comment.
Agree. This is why I mentioned using a longer error message. But it might be fine the way it is
There was a problem hiding this comment.
Hmm, maybe you are right that we should mention that setting slices is not supported?
Doesn't look like it at the moment. Setting slices would be more complicated to implement... |
While we have not made an ultimate decision, my feeling is that we will not support this. |
| def test_setitem_nonstr_key_fails(): | ||
| dg = sc.DataGroup(a=sc.arange('x', 2)) | ||
| with pytest.raises(ValueError): | ||
| dg[0] = sc.scalar(1) |
There was a problem hiding this comment.
Can you add another test, verifying that this is not supported in __init__ either (unless we have that elsewhere already)?
740f958 to
f9109b3
Compare
f9109b3 to
88d2374
Compare
88d2374 to
fc03055
Compare
Fixes #3182.
Is there any case when non str slices are allowed in the
DataGroup.__setitem__method?