deid
deid copied to clipboard
pydicom
0.2.29 rc
Description
@glebsts just opening this so when you have tested and are ready we can merge into master. Definitely no rush, we can do after the holiday! I just want to make sure I don't forget :)
Hello there, sorry for being late, some focus shift. Re this PR: we tested that on our system and figured out that we also need REPLACE to dominate above REMOVE ALL. Without that it became not so useful. We ended up with switching to own implementation with other tech stack, but if you would accept additional commits in this PR with add replace fields to exceptions from remove all, I would gladly make it.
yep, that would work, and just make sure to document everything very clearly so there is no doubt about the functionality and order of things.
Pinging @wetzelj to get his feedback on these changes!
I've been keeping an eye on this issue/pr and am totally on board with the functionality changes/add. At the moment, we've been working on some other additions to our application that uses pydicom/deid, so we've stuck at 0.2.28, but intend to update to latest (and/or this rc) in the next month or so.
Fantastic! So @glebsts go ahead if you want to PR your changes to this branch - we can review / discuss and then get that 0.0.29 finalized.
@vsoch hey, just to clarify - which branch should I should I send PR to (and from which branch better to do it)?
I think if we want it to extend the work here, it would make sense to PR TO 0.2.29-rc. Is there a better way you have in mind?
okay changes from @glebsts are merged when you have some time @wetzelj ! And looks like I can rename the branch, but it will close the PR! Let's finish review, and I'll rename before merge so it's clear.
I think we followed up here with a new PR, so its safe to close. Thanks everyone!
Hi @vsoch - I think this PR was prematurely closed. When I first saw that this was closed, I did notice that #204 had been merged, but on closer review, #198 and #204 were merged into this PR, #204 wasn't a replacement PR that was merged into master.
Since this was closed and not merged, these changes never made it into master (or subsequent branches).
I am currently testing against PR #234. What do you think about the possibility of merging this into that PR? I still believe that there's definite merit to the changes that were introduced here.
We sped past this particular release, so if someone wants to re-base and re-open, I'd be happy to review again! It's mostly an interest thing - 7 months with no response I figured it was outdated or not of interest, sorry about that.
Sounds good. I'll work on it next week. Is it okay if I base it off of #234 or would you prefer it be based off of master?
Whichever is easier to test! Both ultimately are going to be rebase off of master, so choose your pick.