Verify argument type for assigning to Sequence

[suever]

From darcymason@gmail.com on June 23, 2009 18:37:18

Assignment of an item to a Sequence should verify that the item is a Dataset instance, since all sequence items must be one.

So, for example, ds.PatientSetups[0] = "HFP" should give an error.

Steps:

• Add a setitem() method to Sequence which checks using isinstance that the argument is a Dataset instance. If not, raise a "nice" error message, probably ValueError, with a message explaining that Sequence items must be a Dataset
• write unit tests to confirm correct behaviour for good values and bad values.

Original issue: http://code.google.com/p/pydicom/issues/detail?id=52

[suever]

From Suever@gmail.com on December 12, 2011 08:09:56

I was going to take this on, unless you've already started work on this. A couple notes about this:

1) I believe that it should technically throw a TypeError because a ValueError is usually used when the argument is of the correct type, just not the expected value. In this case, the argument would be an incompatible type (not a Dataset)

2) Would you also be interested in overloading init to validate that the constructor is passed valid Datasets as well?

[suever]

From darcymason@gmail.com on December 12, 2011 16:15:12

Suever, sure, it would be great if you could take this on.

1. Agreed. TypeError is more appropriate
2. Yes, excellent suggestion to check in init.

Thanks.

[suever]

From darcymason@gmail.com on January 12, 2012 20:13:58

See also revision c313d2befb08 -- the MultiValue class assures only a certain type allowed in the list -- maybe Sequence can be derived from MultiValue? At least the code can be used as a model.

[suever]

From Suever@gmail.com on January 23, 2012 21:45:18

I had initially created a version of this that basically accomplished the same task as your MultiValue class, but had been thinking about whether Sequence should be derived from a lower level class (i.e. collections.Sequence) to make sure that we can easily validate any changes made to the Sequence. I guess the easiest thing is to just stick with list.

Your MultiValue class looks great. In order to prevent duplicate code, I just subclassed MultiValue and used a validator function in place of the type_constructor.

Unittest is relatively sparse due to the fact that test_multival.py covers most functionality adequately.

Thoughts or suggestions?

-Suever

Attachment: sequence.diff test_sequence.py

[suever]

From Suever@gmail.com on January 23, 2012 21:48:49

Also, the way that it is currently implemented, inputs to the constructor MUST be a list or tuple of Datasets, and it rejects single Datasets:

For example: d = Dataset() seq = Sequence(d)

Throws a TypeError, while seq = Sequence([d,])

Produces the desired Sequence.

The idea was to have a consistent input format for Sequence. All internal uses of Sequence pass iterables of Datasets appropriately.

[suever]

From darcymason@gmail.com on January 24, 2012 06:04:06

Thanks Suever.

I agree with Sequence always taking a list. That fits with python's behaviour and it makes sense for dicom too.

I won't have time to look at this in detail this week, but will try for the weekend or early next week.

Status: Started

[suever]

From darcymason@gmail.com on January 31, 2012 19:20:24

This issue was closed by revision 5a026a1e61e9 .

Status: Fixed