KitwareMedical / dicom-anonymizer

Tool to anonymize DICOM files according to the DICOM standard
BSD 3-Clause "New" or "Revised" License
104 stars 47 forks source link

Fixes for failing unit tests #64

Closed smjoshiatglobus closed 8 months ago

smjoshiatglobus commented 9 months ago

Main changes:

Other changes, mostly for development support:

smjoshiatglobus commented 8 months ago

Just checking... anything you need from me for this pull request?

pchoisel commented 8 months ago

@smjoshiatglobus Thank you for your work and sorry for the delay
I added a few comments to your modifications. Could you have a look and change what is needed ? Thanks !

smjoshiatglobus commented 8 months ago

@smjoshiatglobus Thank you for your work and sorry for the delay I added a few comments to your modifications. Could you have a look and change what is needed ? Thanks !

Thank you, @pchoisel, for the review. I have made a couple of changes and responded to your comments. Please re-review.

pchoisel commented 8 months ago

The changes look good to me ! Could you squash your commits about formatting, spell checking and issue linking ?

smjoshiatglobus commented 8 months ago

The changes look good to me ! Could you squash your commits about formatting, spell checking and issue linking ?

I am trying to learn squashing and worried that I am making it worse! I tried git rebase to squash the three commits on formatting. I am really confused about the commits now! I am not sure if I am doing the squash correctly. Can you please check/help?

pchoisel commented 8 months ago

I can see that the commit number increased instead of decreasing ^^
Here what I do when I want to squash commits:

You can at all moment abort the rebase by doing git rebase --abort At the end you will need to force push.

smjoshiatglobus commented 8 months ago

I can see that the commit number increased instead of decreasing ^^ Here what I do when I want to squash commits:

  • Backup my code in case I need to start again
  • Input git rebase -i HEAD~N => you need to replace N with the number of commits you want to go "back in time". In your case, it will be 31. So input git rebase -i HEAD~31
  • Git will display the 31 last commits in reverse order
  • In front of each commit, you will see a word that will affect what will be done of the commit
  • All you need to do is replace keep with squash for the commit you wish to squash and save the file

You can at all moment abort the rebase by doing git rebase --abort At the end you will need to force push.

I create a separate branch to test this. I followed the steps you mentioned above. Instead of keep, I saw the word pick, which I retained.

There were several merge conflicts that I had to resolve. I have created draft PR #65 to show the changes. It still looks very clumsy! Sorry, I still don't know why there were so many merge conflicts.

I also created draft PR #66 with squashed commits. I am fine if you prefer to merge PR #66 and close this one.

pchoisel commented 8 months ago

Code merged in #66