dcmjs-org / dcmjs

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

feat(anonymizer) - export Array tagNamesToEmpty and modify cleanTags #303

Closed juliencastelneau closed 1 year ago

juliencastelneau commented 2 years ago
juliencastelneau commented 1 year ago

Hello, Is there an issue with the PR ? @pieper I made the changes you requested. Maybe I do not understand something. Do you want another modification ? thx

pieper commented 1 year ago

Thanks for the contribution and the reminder 👍

pieper commented 1 year ago

@juliencastelneau it looks like there's at least one error due to the anonymizer - could you have a look?

https://github.com/dcmjs-org/dcmjs/actions/runs/3047634658/jobs/4911834982#step:5:2486

juliencastelneau commented 1 year ago

The test fails because I added an argument to the method cleanTags I think there are 2 ways to solve.

  1. Modify the test:

Add the lines below in anonymizer.test.js (line 26)

    var tagsToReplace = {
        "00100010":   "ANON^PATIENT",
    }

    // when
    cleanTags(dicomDict.dict, tagsToReplace);

instead of

    cleanTags(dicomDict.dict);
  1. Modify the method cleanTags to ensure the behaviour is unchanged:
export function cleanTags(
    dict,
    tagNamesToReplace = undefined,
    customTagNamesToEmpty = undefined
) {

    if (tagNamesToReplace==undefined) {
         tagNamesToReplace = {
            "00100010":   "ANON^PATIENT",
            "00100020":   "ANON^ID"
         }
    }

    var tags =
        customTagNamesToEmpty != undefined
            ? customTagNamesToEmpty
            : tagNamesToEmpty;

@pieper The best way is to modify the method cleanTags I guess. Do I need to make a new PR ?

pieper commented 1 year ago

Thanks for looking into this. I think the second option is better since it maintains backwards compatibility for the API. We should also add a new test demonstrating the ways to use the method and confirm it keeps working.

Yes, a new PR would be great since this has already been merged.