-
Notifications
You must be signed in to change notification settings - Fork 228
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
base: master
Are you sure you want to change the base?
Conversation
- 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
Come to think about it, the introduction of the Tag type might be a breaking change. Maybe should remove that. |
@Ragnar-Oock Can I get push rights to your branch so we can collaborate on it? |
Can you give me an example of where it would break something? |
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 A typescript playground demonstrating the problem |
I thought about it a bit more. While having the 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
I'm not sure why the tests are failing, it says it can't install the ChromeDriver. Could someone look into that? |
Yeah, please rebase onto yesterday's master--Circle CI's config needed updating per some changes the Chrome team made that broke things. |
@yagni I just seen that you never accepted the invite for the collaborator access you've asked, I renewed the invite |
This PR is a follow-up to #257
Changes done in this PR :
Element
in the various types that actually exist are run time (DataElement, Sequence, EncapsulatedPixelDataElement, FileMetaInformationElement)Notes :
Tag
and narrowed downElement
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.