Skip to content
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

Fixes for failing unit tests #64

Closed

Conversation

smjoshiatglobus
Copy link
Contributor

@smjoshiatglobus smjoshiatglobus commented Jan 26, 2024

Main changes:

Other changes, mostly for development support:

  • Added references to known issue X tags of VR == 'DA' not getting deleted #56 in tests code
  • Added Pipfile for optional development setup using pipenv
  • Added configuration for spell-checking using cspell and added custom dictionary.
  • Fixed a typo (only one!)
  • Formatted code in tests and example folders using black
  • Suppressed some warnings from Sourcery

@smjoshiatglobus
Copy link
Contributor Author

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

dicomanonymizer/simpledicomanonymizer.py Outdated Show resolved Hide resolved
dicomanonymizer/simpledicomanonymizer.py Show resolved Hide resolved
Comment on lines 30 to 32
replaced_failed = []
deleted_failed = []
emptied_failed = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fond of removing tests because they fail. Let's leave them fail to remind us of the problem that needs fixing.
If this prevents merging, I'll change the related setting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! These were remnants from code no longer needed, since all (unexpectedly) failing tests are now removed. I have removed these lines in 2d53d34

Comment on lines +57 to +59
if (
len(tt) == 2 and tt in orig_ds
): # sourcery skip: merge-nested-ifs, no-conditionals-in-tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formatting feels weird, is it because of Flake8 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment suppresses couple of errors from Sourcery plugin: https://sourcery.ai/

@pchoisel
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@smjoshiatglobus
Copy link
Contributor Author

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
Copy link
Collaborator

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.

@smjoshiatglobus
Copy link
Contributor Author

smjoshiatglobus commented Feb 29, 2024

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
Copy link
Collaborator

Code merged in #66

@pchoisel pchoisel closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants