Skip to content

Minor changes to DirectoryHelper.DeleteDirectory() #977

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 24, 2015
Merged

Minor changes to DirectoryHelper.DeleteDirectory() #977

merged 1 commit into from
Feb 24, 2015

Conversation

nulltoken
Copy link
Member

This builds up on #971 and proposes the following modifications

  • Allow derived exceptions from known ones to be whitelisted
  • Trace the number of failed attempts made to delete the directory
  • Use named parameters to make the retry parameters less "magic" to the reader

@nulltoken
Copy link
Member Author

/cc @ThomasBarnekow @jamill

ThomasBarnekow
ThomasBarnekow reviewed Feb 23, 2015
View reviewed changes
LibGit2Sharp.Tests/TestHelpers/DirectoryHelper.cs
if (!whitelist.Contains(ex.GetType()))
var exceptiontype = ex.GetType();

if (!whitelist.Select(e => exceptiontype.IsAssignableFrom(e)).Any())
Copy link
Contributor

Choose a reason for hiding this comment

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

Like!

Copy link
Member

Choose a reason for hiding this comment

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

Except it's a verbose way of saying:
!whitelist.Any(e => exceptiontype.IsAssignableFrom(e))

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@nulltoken
Copy link
Member Author

@ThomasBarnekow Can you fetch this and make this I didn't break anything, please?

@ThomasBarnekow
Copy link
Contributor

@nulltoken Will do.

@ThomasBarnekow
Copy link
Contributor

@nulltoken The expression must be turned around like so:

if (!whitelist.Any(e => e.IsAssignableFrom(exceptionType)))

Otherwise, derived exceptions (e.g., our beloved DirectoryNotFoundException) will be thrown instead of caught.

@nulltoken
Copy link
Member Author

@nulltoken The expression must be turned around like so:

Fixed.

YU SO MEAN IsAssignableFrom()

@ThomasBarnekow
Copy link
Contributor

@nulltoken Just ran all the tests. 808 passed. 0 failed. 7 skipped. All good!

I'd have two thoughts which, however, don't stand in the way of merging this fix:

  • First, wouldn't it still make sense to catch exceptions in the BaseFixture.Dispose() method (as DeleteDirectory() could theoretically throw)? Or should the top-level DeleteDirectory() method catch unexpected exceptions and write trace messages for these as well? Currently, these unexpected exceptions (if they can ever happen) would lead to failed tests. You might want that but you might as well not.
  • Second, would it make sense to rename the top-level DeleteDirectory() method into TryDeleteDirectory() as well? My rationale is that it (1) does not ensure directories are indeed deleted and (2) does not provide any direct feedback to the caller (only BaseFixture.Dispose() at the moment) that it could not delete a directory.

@nulltoken
Copy link
Member Author

@ThomasBarnekow Those are good points

Currently, these unexpected exceptions (if they can ever happen) would lead to failed tests.

I actually think I'd prefer them to fail the test. The noise would raise the chance for a user to open an issue about this and give us an opportunity to analyze/troubleshoot this unexpected situation.

Would it make sense to rename the top-level DeleteDirectory() method into TryDeleteDirectory()

Why not? However, by convention, TryXxxx methods expose a certain signature (a returned bool and an in out param). I'd prefer using a name that doesn't conflict with a known pattern.

Just ran all the tests. 808 passed. 0 failed. 7 skipped. All good!

🆒 Thanks for having checked this 💖 I'm going to merge this (after a quick rebase). Let's discuss the DeleteDirectory naming proposal in a separate issue.

@nulltoken
Minor changes to DirectoryHelper.DeleteDirectory()
e1228da 
 - Allow derived exceptions from known ones to be whitelisted
 - Trace the number of failed attempts made to delete the directory
 - Use named parameters to make the retry parameters less "magic" to the reader
@nulltoken nulltoken force-pushed the ntk/directory_delete branch from bc47582 to e1228da Compare February 24, 2015 18:55
@ThomasBarnekow
Copy link
Contributor

@nulltoken I agree with all of your points.

@nulltoken nulltoken merged commit e1228da into vNext Feb 24, 2015
@nulltoken nulltoken deleted the ntk/directory_delete branch February 24, 2015 19:46
@nulltoken nulltoken mentioned this pull request Feb 24, 2015
Closed
@nulltoken nulltoken added this to the v0.22 milestone Mar 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.22
Development

Successfully merging this pull request may close these issues.
3 participants
@nulltoken @ThomasBarnekow @Therzok