Skip to content
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

Improve type declaration #258

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ragnar-Oock
Copy link

This PR is a follow-up to #257

Changes done in this PR :

  • split Element in the various types that actually exist are run time (DataElement, Sequence, EncapsulatedPixelDataElement, FileMetaInformationElement)
  • improve type for VR and tag parameters
  • type Type Guard functions as such

Notes :

  • The declaration for DataSet and ByteArrayParser has been more or less taken as is, I just updated the elements property and added missing JSDoc on .byteArray and .byteArrayParser.
  • I added the JSDoc with my knowledge of the DICOM spec, how the lib tends to parse the files I throw at it, and some referencing to the code. Still, I can't guarantee the exactness of it and would appreciate it if someone with more knowledge took a look at it to validate.
  • The VR enum is extracted from the table in DICOM Part 5 Section 6.2, I considered adding links to each VR's definition in this table but there is no id to anchor to in the page from NEMA.
  • While I thought about updating the declaration for the various pixel data helper functions and some of the sequence reader functions to use the new Tag and narrowed down Element I decided against it as I don't have a good enough understanding of how they work and what kind of data they are actually expecting. This is thus something that can be further improved.

- split Element in the various types that actually exist are run time
- improve type for VR and tag parameters
- type Type Guard functions as such
@Ragnar-Oock
Copy link
Author

Come to think about it, the introduction of the Tag type might be a breaking change. Maybe should remove that.

@yagni
Copy link
Collaborator

yagni commented Aug 7, 2023

@Ragnar-Oock Can I get push rights to your branch so we can collaborate on it?

@yagni
Copy link
Collaborator

yagni commented Aug 8, 2023

Come to think about it, the introduction of the Tag type might be a breaking change. Maybe should remove that.

Can you give me an example of where it would break something?

@Ragnar-Oock
Copy link
Author

Ragnar-Oock commented Aug 8, 2023

If someone does something like :

const TAG_I_AM_INTERESTED_IN = 'x00200000'; //random tag, the actual value does not matter

const element = dataset.elements[TAG_I_AM_INTERESTED_IN];

They would get an error from TS saying that string can't be used to index type {[x: `x${string}`] : Element}.
And it doesn't add much anyway, so if you want to keep it maybe add it to the list of stuff to do in v2. Plus, the fact we can't limit the length of the string or the chars inside to be an actual 4 bytes hexadecimal value makes it a bit pointless.

A typescript playground demonstrating the problem

@Ragnar-Oock
Copy link
Author

I thought about it a bit more.

While having the Tag type as it is now is a breaking change, it could be mitigated by changing it from :

type Tag = `x${string}`;

to

type Tag = `x${string}` | string;

That way it's possible to easily distinguish in the API what expects/returns a tag and what expects/returns just a string. And it's possible to have a doc string on it directly for intellisence to show to people that are not savvy of how the lib or the DICOM spec function (newcomers to a project using the lib mostly)

make breaking change of Tag type retro compatible with old interface
@Ragnar-Oock
Copy link
Author

I'm not sure why the tests are failing, it says it can't install the ChromeDriver. Could someone look into that?

@yagni
Copy link
Collaborator

yagni commented Aug 22, 2023

Yeah, please rebase onto yesterday's master--Circle CI's config needed updating per some changes the Chrome team made that broke things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants