Re: Issue 52 in pydicom: Verify argument type for assigning to Sequence

[pyd...@googlecode.com]

Comment #1 on issue 52 by Sue...@gmail.com: Verify argument type for
assigning to Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

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?

[pyd...@googlecode.com]

Comment #2 on issue 52 by darcy...@gmail.com: Verify argument type for

assigning to Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

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.

[pyd...@googlecode.com]

Comment #3 on issue 52 by darcy...@gmail.com: Verify argument type for

assigning to Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

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.

[pyd...@googlecode.com]

Comment #4 on issue 52 by Suever: Verify argument type for assigning to
Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

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

Attachments:
sequence.diff 2.4 KB
test_sequence.py 2.1 KB

[pyd...@googlecode.com]

Comment #5 on issue 52 by Suever: Verify argument type for assigning to
Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

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.

[pyd...@googlecode.com]

Updates:
Status: Started

Comment #6 on issue 52 by darcy...@gmail.com: Verify argument type for

assigning to Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

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.

[pyd...@googlecode.com]

Updates:
Status: Fixed

Comment #7 on issue 52 by darcy...@gmail.com: Verify argument type for

assigning to Sequence
http://code.google.com/p/pydicom/issues/detail?id=52

This issue was closed by revision 5a026a1e61e9.