search

dcmjs-org / dcmjs

Javascript implementation of DICOM manipulation
https://dcmjs.netlify.com/
MIT License
292 stars 111 forks source link

Refactor and fragment bug #283

Closed Ouwen closed 2 years ago

Ouwen commented 2 years ago

Fix for #282.

Also refactors options into dictionary params to maintain public API. Moved optional parameter tests into separate data-options.test.js Removed legacy untilTag.test.js

pieper commented 2 years ago

Does this change break the API?

Ouwen commented 2 years ago

It shouldn’t break the API (DicomMessage.readFile)

Ouwen commented 2 years ago

@pieper the refactor commit will change the function signature of

DicomMessage.read and DicomMessage.readTag

However, my understanding is that these are largely used internal to the dcmjs package. I've done a sweep to edit any place they are used (tests).

The change puts options like ignoreErrors, untilTag, and includeUntilTagValue, into an options param. Future PRs which add options can be made without modifying the function signature.

pieper commented 2 years ago

Yes, I agree, they are unlikely to be used outside of the parsing code. I wonder if the "correct" thing to do would be to change the names of these and similar internal methods to start with an underscore and leave the existing ones as-is but mark them as deprecated. What do you think?

Ouwen commented 2 years ago

I think that makes sense and would be a good notification for anyone still using it that it'll be dropped in the next major version release. I can put up a quick PR for that.

Ouwen commented 2 years ago

@pieper let me know if this looks good to you

pieper commented 2 years ago

Thanks for this 👍 keeping the API stable makes me more comfortable.

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 0.24.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket: