Verify argument type for assigning to Sequence

[GoogleCodeExporter]

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 reported on code.google.com by darcymason@gmail.com on 23 Jun 2009 at 10:37

[GoogleCodeExporter]

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?

Original comment by Suever@gmail.com on 12 Dec 2011 at 4:09

[GoogleCodeExporter]

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.

Original comment by darcymason@gmail.com on 13 Dec 2011 at 12:15

[GoogleCodeExporter]

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.

Original comment by darcymason@gmail.com on 13 Jan 2012 at 4:13

[GoogleCodeExporter]

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

Original comment by Suever@gmail.com on 24 Jan 2012 at 5:45

Attachments:

• sequence.diff
• test_sequence.py

[GoogleCodeExporter]

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.

Original comment by Suever@gmail.com on 24 Jan 2012 at 5:48

[GoogleCodeExporter]

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.

Original comment by darcymason@gmail.com on 24 Jan 2012 at 2:04

• Changed state: Started

[GoogleCodeExporter]

This issue was closed by revision 5a026a1e61e9.

Original comment by darcymason@gmail.com on 1 Feb 2012 at 3:20

• Changed state: Fixed