-
Notifications
You must be signed in to change notification settings - Fork 53
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
[ENH] Updated clear_drives function and added tests #612
Conversation
@raj1701 you have a conflict in |
all_drives = net.get_external_drive_names() | ||
drives_to_be_deleted = all_drives[0:2] # Deleting 2 drives in this test | ||
net.clear_drives(drive_names=drives_to_be_deleted) | ||
assert len(net.connectivity) == 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do you get this number 15 from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just printed net.connectivity values and got it from there. But I will check out what net.connectivity is and figure out another way to find the values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pick_connection
may be helpful. You basically want to check how many connections the drive had before and subtract that from the total number of connections.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote my own function for getting connections of an external drive but will move to pick connection.
Also I found a bug in clear_connectivity function. Basically it will work correctly when no arguments are passed, but when drive names are passed to it, it deletes all connections other than the ones which were intended. Basically reverse behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be more clear once I push the commit.
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #612 +/- ##
==========================================
- Coverage 92.03% 91.97% -0.07%
==========================================
Files 22 22
Lines 4256 4260 +4
==========================================
+ Hits 3917 3918 +1
- Misses 339 342 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@rythorpe I will try to rebase on my own. If I get stuck will ask for your help. |
Hey @rythorpe @jasmainak sorry I got stuck in rebasing and it got messed up a lot. Can I make a new branch and push everything. |
Sorry for all the mishap. I have opened another pull request for the issue |
Normally when this happens, it is better to rename the current branch, create a new branch with the same name and then force push: $ git branch -m delete_drive_old
$ git checkout master
$ git branch delete_drive
$ git checkout delete_drive then add your changes to this branch, and finally: $ git push -f origin delete_drive |
Thanks. I will keep this in mind |
replaced by #613 |
Closes #549
I cherry picked the earlier commit to clear_drives.
Added tests to delete a custom number of drives and all drives
make test passed in local setup