-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make DicomValue a homogeneous collection instead of heterogeneous #13
base: main
Are you sure you want to change the base?
Conversation
Looks awesome, thanks for the PR. I've run the tests and some errors are occuring.
36/195 are failing so seeing what is common among those should make fixing this easy. Merging this PR is not a massive priority at the moment, so I'll debug this one when I get a chance, possibly next week. |
This is what each of the tracebacks look like.
Error messages look very nice :) |
Very odd. Are those tests that are not run via
|
Ah! I have to run |
The previous type allowed for a collection of mixed integers / strings / floats, etc. Since the DICOM format doesn't allow for that, we can change the type to enforce a single inner type. We also create our own `Value` enum instead of using `rmpv::Value`. This allows us to ignore non-UTF8 strings and integers that cannot be represented as i64.
5aac4c0
to
d65a2a3
Compare
OK, should be fixed now. I got confused and thought that both |
The previous type allowed for a collection of mixed integers / strings
/ floats, etc. Since the DICOM format doesn't allow for that, we can
change the type to enforce a single inner type.
We also create our own
Value
enum instead of usingrmpv::Value
. This allows us to ignore non-UTF8 strings and integersthat cannot be represented as i64.