naturalizeDataset make sequences of length 1 into an object, which makes it hard to write generic code - replaced with #219

[wayfarer3130]

If there is a sequence which is sometimes of length 1 (eg based on number of frames being 1), then it is difficult to write code that generically gets the correct sequence number, for example metadata.PerFrameFunctionalGroupsSequence[frameNumber-1] doesn't work correctly.

A trivial fix for this is to replace the line: naturalDataset[naturalName] = naturalDataset[naturalName][0]; with naturalDataset[naturalName] = Object.assign(naturalDataset[naturalName],naturalDataset[naturalName][0]);

This simply ensures that the object is both a list (array) and an object, so it can then just be used as either type.

[pieper]

Yes, I thought we can cleaned that up at one point and always represented sequences as lists even with only one element. Can you make a PR with your suggested changes for discussion? As a general rule it's turned out to be best to stay as close as possible to the semantics of the standard. It could be worth making a breaking change as a new major version and make sure the client and server code are also updated and tested.

[wayfarer3130]

I tried creating a PR, but have something wrong in it and github won't let me create it. Anyways, the fork/diff is here: Comparing commontk:master...wayfarer3130:fix-array-sequences-3 · commontk/dcmjs (github.com) https://github.com/commontk/dcmjs/compare/master...wayfarer3130:fix-array-sequences-3?expand=1 If you can tell me how to correct whatever I did wrong, then I will try to fix it, but several of the suggestions I saw online just didn't work.

Bill

On Tue, 14 Sept 2021 at 09:16, Steve Pieper @.***> wrote:

Yes, I thought we can cleaned that up at one point and always represented sequences as lists even with only one element. Can you make a PR with your suggested changes for discussion? As a general rule it's turned out to be best to stay as close as possible to the semantics of the standard. It could be worth making a breaking change as a new major version and make sure the client and server code are also updated and tested.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dcmjs-org/dcmjs/issues/218#issuecomment-919140945, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGT56XJKVJAYKA2OLVHOGXLUB5DKXANCNFSM5EACUDOQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[pieper]

It looks like your fork of the repository is not up to date with the current master. Maybe you can re-fork and copy in the changes to make a clean PR.

I was going to say though that perhaps this whole block should be removed and we'd get the expected behavior.

https://github.com/commontk/dcmjs/compare/master...wayfarer3130:fix-array-sequences-3?expand=1#diff-a70070702108a92f492970cff5b5ef645d5c33c56c01a775d65063e9bdbefa7aR144-R150

[ladeirarodolfo]

I just checked this issue, not sure what exactly @wayfarer3130 is your fix. But I know what should be avoided

https://github.com/dcmjs-org/dcmjs/blob/master/src/DicomMetaDictionary.js#L148

we can find here https://262.ecma-international.org/6.0/#sec-object.assign But Briefly explanation: Object.assign(a, [a]) and a = [b] => Object.assign([b], [[b]])

1. [b] is already an object.
2. It will copy all enumerable properties from source. In this case properties of object a
3. Every time we copy a new property we are actually adding more properties to [b], because target and source has same memory reference. I.e it will add a circular dependency here.

Not sure what exactly that line (L148) is intended to do, I just would like to share my thoughts here.

[wayfarer3130]

Replaced with issue 219